Move platform theming logic from button component into theme #3627
Conversation
With some platform specific Platform Colors, I think that was explicitly not a goal when I touched this last. as an fyi |
|
Second comment. Moving files from platform specific to not platform specific will mean increased bundle size since metro won't filter them out. I'm not sure that's an issue anymore, but it was the reason we went for platform specific files for tokens everywhere. |
I dont think any production bundles are going to be including the android theme on win32 for example. However, there will potentially be a larger bundle for users that are only using one component, which will now get a bundle including the theme for all components. |
packages/theming/android-theme/src/components/Button/ButtonTheme.ts
Outdated
Show resolved
Hide resolved
packages/theming/apple-theme/src/components/Button/FABTokens.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // grab the styled slots | ||
| const Slots = useSlots(userProps, (layer) => buttonLookup(layer, toggleButton.state, userProps)); | ||
| const Slots = useSlots(userProps, (layer) => buttonLookup(layer, toggleButton.state, userProps, theme)); |
There was a problem hiding this comment.
Huh. I'm surprised this needed to be manually added.
There was a problem hiding this comment.
Surprised that it wasn't already there.. or surprised / questioning the need for the change?
| export const defaultButtonTheme = (theme: Theme) => | ||
| ({ | ||
| components: { | ||
| Button: immutableMerge<object>( |
There was a problem hiding this comment.
If you can, you should probably pull the names here from the exported buttonName, etc. from the component
There was a problem hiding this comment.
default-theme cannot depend on Button, because Button depends on framework which depends on default-theme. :(
|
|
||
| export const defaultButtonTheme = (theme: Theme) => | ||
| ({ | ||
| components: { |
There was a problem hiding this comment.
What's the thinking for the older version of the Button? Although it's marked as deprecated we haven;t been able to force everyone off of it yet.
There was a problem hiding this comment.
Do you think we have users outside of win32?
I had to move GDPR to the new button because the older version was relying on some layout behavior we have in win32 that isn't in the other RN platforms. So, the older button would layout incorrectly when I switched to fabric.
So, I guess I'm suggesting forcing people to upgrade before they can move to fabric.
Currently there is a lot of hidden platform checks in many of our components, which control the appearance outside of the theme. This essentially means that our themes only work on their specific platform.
This change starts to move logic out of internal platform checks and into the theme objects. Eventually this will allow themes to work better across platforms. It also ensures that themes have maximum capabilities. Internally we will need this as we will want to use the win32-theme on the windows platform when we move to fabric.