-
Notifications
You must be signed in to change notification settings - Fork 2
Conversation
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.
Consider comments and merge.
src/components/Provider/Provider.tsx
Outdated
@@ -94,7 +94,7 @@ class Provider extends Component<any, any> { | |||
|
|||
// ensure we don't assign `undefined` values to the theme context | |||
// they will override values down stream | |||
const theme: any = {} | |||
const theme: any = { isRtl: !!this.props.rtl } |
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.
Please destructure rtl
at the top of the function with the other props.
I propose we don't rename props as they pass from the Provider to the theme. It adds unnecessary thinking to follow renames.
Yep, this is required. Please see comment and then merge. |
comments are addressed |
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
=======================================
Coverage 69.94% 69.94%
=======================================
Files 74 74
Lines 1198 1198
Branches 227 227
=======================================
Hits 838 838
Misses 355 355
Partials 5 5
Continue to review full report at Codecov.
|
This change is necessary for the sake of component styles being able to adapt to current presence of RTL mode.
As an example, suppose that we have a Chat Message component with the following layout
Relevant thing for us is the fact that 'timestamp' region has left padding, to ensure it won't overlap content of the 'author' area. However, in the RTL mode we would like this padding being applied on the right side of the component, not still on the left one:
But, unfortunately, at the moment we are unable to detect whether RTL is enabled from the 'rules' function. What would solve the problem is that if we would pass
isRtl
flag to rules functions, such that the following will be possible: