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
Fabric: Properly handle dir being passed into control #11626
Conversation
Component Perf AnalysisNo significant results to display. All results
|
@dzearing are you good with this change? |
const divProps = getNativeProps<React.HTMLAttributes<HTMLDivElement>>(this.props, divProperties, ['dir']); | ||
// If dir is set via props, use that value, otherwise if theme is set, use theme RTL setting, otherwise, set undefined. | ||
const htmlDir = | ||
dir !== undefined ? dir : theme !== undefined && theme.rtl !== undefined ? (theme.rtl === true ? 'rtl' : 'ltr') : undefined; |
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.
minor nit but I think this could be a little more readable:
const themeDir = (theme && theme.rtl) ? 'rtl' : 'ltr';
const htmlDir = dir !== undefined ? dir : themeDir;
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.
minor nit.
per david's suggestion, the dir logic is a bit more simple and will always return RTL or LTR for a Fabric component. |
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 61d9d19cb20855cc6d2930a4764ff397a3cf1bc4 (build) |
had to do a bit more work to account for page level RTL (which does not affect theme). Reworked a bit of my and Davids Customizer logic to be a bit easier to follow. Specifically here: https://github.com/OfficeDev/office-ui-fabric-react/pull/11626/files#diff-3b229796dd0781151c4cb54697913f02R21 |
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.
Looks like the snaps just need to be updated
🎉 Handy links: |
Pull request checklist
Description of changes
Earlier Fabric component update set
direction: rtl
based off of the theme alone. This update will look first to thedir
attribute (if provided), and then look to the theme.rtl (if provided) before settingdir
attribute of the Fabric component.Updated tests as well.
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow