-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Provider): Make Provider a UIComponent #852
Conversation
Codecov Report
@@ Coverage Diff @@
## master #852 +/- ##
==========================================
+ Coverage 82.08% 82.11% +0.02%
==========================================
Files 713 716 +3
Lines 8536 8559 +23
Branches 1161 1230 +69
==========================================
+ Hits 7007 7028 +21
- Misses 1513 1515 +2
Partials 16 16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/react/test/specs/components/Provider/Provider-test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. changes in mergeRTL()
This is a second point of my confusion.
<Provider theme={{}} />
<Provider theme={{}} /> // rtl: false
// ---
<Provider theme={{}} />
<Provider theme={{ rtl: true }} /> // rtl: true
// ---
<Provider theme={{ rtl: true }} />
<Provider theme={{}} /> // rtl: true
Why it should be undefined
? It will add dir='ltr'
to the root node, but why this is not correct?
Fela uses direction
prop on theme
, robinweser/fela#673. I think that we should do the same thing instead of rtl: boolean
.
The original code works for merging themes in teamsTheme = mergeThemes(base, {/*...*/}) RTL is defined neither in base theme nor in teams overrides but with the original code |
The reason to add
The reason why we cannot do that directly in As discussed offline with @layershifter the main problems of the approach taken are:
Alternative proposal (@layershifter) is:
|
First shipment should be as minimal as possible. We won't advertise opting out of the styled div until we get requests. Even then, the initial workaround suggestion will be |
@@ -19,6 +19,7 @@ class App extends React.Component<any, ThemeContextData> { | |||
return ( | |||
<ThemeContext.Provider value={this.state}> | |||
<Provider | |||
as={React.Fragment} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary hotfix for Docsite when switching themes for examples. We should fix theme switching to apply to examples only, not to the whole docsite.
<div | ||
dir={theme.rtl ? 'rtl' : undefined} | ||
style={{ | ||
color: siteVariables.bodyColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted #1050, removed unnecessary div.
BREAKING CHANGE
<Provider>
component now renders a<div>
element withdir
attribute and styles settingbackground
andcolor
CSS attributes.Fixes #586.
Change description
<Provider>
component now creates a<div>
element.Provider
can now contain multiple childrenBefore
After
Theme colors (
bodyBackground
andbodyColor
) are correctly applied for all childrenCode
Before
After
There is also
dir
attribute correctly set on thediv
so RTL works without any additional changes:Before
After
If
rtl
is not defined in the nestedProvider
, it is now inherited from the outer providerIt is possible to override variables and styles for the
ProviderBox
After
Known limitations
Docsite displays
div
created by the example-wrapping Provider as part of the Rendered HTML.