Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend theme* props to accept functions that receive the default class list #26

Open
davidtheclark opened this issue Aug 2, 2018 · 2 comments

Comments

@davidtheclark
Copy link
Contributor

Having experimented with a few different options, here's the outline of a styling pattern that I think could fit our purposes:

  • Opinionated defaults.
  • Standard variants are easy to implement, with either boolean or keyword props. The current MbxButton component illustrates this approach with the variant, size, and width props.
  • Non-standard variants are possible to implement, with theme* props. This is the pattern established by the bulk of existing components and has proven useful. The main problem with it is that it requires a lot of the user — so it makes sense for advanced users, but not for the average user. We should continue to use these but indicate in the docs that they are advanced props and most people should not need to use them.

One major annoyance with the theme* props is that you need to replace every class that you don't want to override. A common use case is to just append a class to the end of the list, but to do this you actually need to copy-paste the default class list and add your own to the end — and that's not very robust or cheery. So I'm thinking that instead of theme* props accepting only strings, they'll also accept functions that receive the default class list and return a modified one. Appending would be very easy, then.

cc @danswick

@davidtheclark
Copy link
Contributor Author

@kepta helped come up with an adjustment to this that will probably be nice.

Since the most common use case for theme* props is probably to append a class or two, and only the Very Advanced User would want to replace the whole class list or remove parts of it, we can make that primary use case easier: If you provide a string to a theme* prop, it will be appended to the class list. If you provide a function, you can do whatever you want, wholly replacing the class list with whatever you return.

@davidtheclark
Copy link
Contributor Author

I audited our use of theme* props in major codebases, and I think now that we should not keep them around. Here's what I think transition off of them would look like:

  • Common style variations will be built into the component.
  • Layout can be determined with a wrapper component. Lots of <Icon themeIcon="..." /> usage, for example, is just to achieve layout that could be done with a wrapper.
  • If complete customization is really necessary and making a custom component isn't in the cards, because the functionality is complex, we'll try to expose unstyled low-level components. Essentially following the strategy of react-aria-modal, where you have to bring all your own styles. For example, we could expose both a styled and an unstyled tooltip or popover. That would come with the caveat that the unstyled one would rely on your completely for its styling.
  • If you want a thoroughly unique design (like some inputs on some marketing pages) you should probably make a custom component.
  • ControlSwitch will have better-styled labels built in.
  • IconButton is a big dumb problem that should be rethought.
  • ToggleGroups have real styling issues that I don't think I'm going to confront — so if you are a ToggleGroup advocate, I'd appreciate your help.

cc @mapbox/frontend

@dereklieu dereklieu added the p2 label Apr 27, 2021
@tristen tristen changed the title Tiered styling properties Extend theme* Props to accept functions that receive the default class list Jul 28, 2022
@tristen tristen changed the title Extend theme* Props to accept functions that receive the default class list Extend theme* props to accept functions that receive the default class list Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants