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

fix: update RTL in ax(), useAx() removal #17713

Merged
merged 6 commits into from
Apr 7, 2021
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Apr 6, 2021

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Why?

I tried to use useAx() in rest of codebase and discovered a problem, we use ax() to merge classes conditionally, but React hooks can't be conditional:

if (state.children) {
  state.children.className = useAx(); // 💣 breaks order of hooks
}

Description of changes

This PR reworks changes that we introduced in #17549. A context subscription was required to support cross-boundary scenarios (passing overrides between providers with different text directions, check removed stories). However, during offline conversations we agreed that we can drop this scenario as it's not required. I added an error in development to notice consumers that this scenario is not supported.

ax() signature reverted back

The signature of ax() function @fluentui/make-styles have been reverted back:

// Before
ax("ltr", [className1, className2]);
// After
ax(className1, className2);

useAx() is removed

#17549 introduced useAx() in @fluentui/react-make-styles, it consumed a React Context and was a React hook. I reverted this change:

// Before
useAx(className1, className2);
// After
ax(className1, className2);

@@ -150,29 +146,4 @@ storiesOf('MakeStyles', module)
<Container primary>Hello world!</Container>
</FluentProvider>
</FluentProvider>
))
.addStory('RTL: propagation of styles via providers with different directions', () => (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support this scenario.

@@ -9,10 +9,10 @@ import {
MakeStylesStyleRule,
} from './types';

type Created<Slots extends string> = Record<Slots, Record<string, MakeStylesResolvedRule>>;
type ResolvedStylesBySlots<Slots extends string> = Record<Slots, Record<string, MakeStylesResolvedRule>>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type missed proper naming 😿

@size-auditor
Copy link

size-auditor bot commented Apr 6, 2021

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-components-makeStyles 22.239 kB 22.251 kB ExceedsBaseline     12 bytes
office-ui-fabric-react fluentui-react-components-Menu 66.78 kB 66.632 kB BelowBaseline     -148 bytes
office-ui-fabric-react fluentui-react-components-Avatar 45.703 kB 45.555 kB BelowBaseline     -148 bytes
office-ui-fabric-react fluentui-react-components-Link 33.301 kB 33.127 kB BelowBaseline     -174 bytes
office-ui-fabric-react fluentui-react-components-Image 33.067 kB 32.893 kB BelowBaseline     -174 bytes
office-ui-fabric-react fluentui-react-components-Divider 33.319 kB 33.145 kB BelowBaseline     -174 bytes
office-ui-fabric-react fluentui-react-components-Button 38.371 kB 38.197 kB BelowBaseline     -174 bytes
office-ui-fabric-react fluentui-react-components-ax 1.525 kB 1.256 kB BelowBaseline     -269 bytes

ExceedsTolerance Over Tolerance (1024 B) ExceedsBaseline Over Baseline BelowBaseline Below Baseline New New Deleted  Removed 1 kB = 1000 B

Baseline commit: 0b50201e6e8469298bbc9472161fe51bcaaff6f1 (build)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 6, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 48b970c:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Apr 6, 2021

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1016 1031 5000
BaseButton mount 1016 989 5000
Breadcrumb mount 46488 45849 5000
ButtonNext mount 630 629 5000
Checkbox mount 1697 1721 5000
CheckboxBase mount 1397 1437 5000
ChoiceGroup mount 5285 5192 5000
ComboBox mount 1043 1077 1000
CommandBar mount 10583 10894 1000
ContextualMenu mount 6774 6664 1000
DefaultButton mount 1224 1272 5000
DetailsRow mount 4063 3892 5000
DetailsRowFast mount 3924 4045 5000
DetailsRowNoStyles mount 3676 3731 5000
Dialog mount 1572 1541 1000
DocumentCardTitle mount 1871 1879 1000
Dropdown mount 3538 3543 5000
FocusTrapZone mount 1900 1852 5000
FocusZone mount 1869 1882 5000
IconButton mount 1874 1926 5000
Label mount 356 364 5000
Layer mount 1963 1973 5000
Link mount 507 512 5000
MakeStyles mount 1811 1845 50000
MenuButton mount 1593 1561 5000
MessageBar mount 2177 2178 5000
Nav mount 3637 3639 1000
OverflowSet mount 1107 1106 5000
Panel mount 1572 1572 1000
Persona mount 896 925 1000
Pivot mount 1538 1553 1000
PrimaryButton mount 1399 1425 5000
Rating mount 8495 8657 5000
SearchBox mount 1481 1457 5000
Shimmer mount 2753 2862 5000
Slider mount 2164 2133 5000
SpinButton mount 5386 5358 5000
Spinner mount 439 437 5000
SplitButton mount 3410 3506 5000
Stack mount 531 528 5000
StackWithIntrinsicChildren mount 1730 1708 5000
StackWithTextChildren mount 5217 5164 5000
SwatchColorPicker mount 11124 11176 5000
Tabs mount 1591 1523 1000
TagPicker mount 3169 3188 5000
TeachingBubble mount 12501 12482 5000
Text mount 466 492 5000
TextField mount 1507 1525 5000
ThemeProvider mount 1257 1258 5000
ThemeProvider virtual-rerender 632 649 5000
ThemeProviderNext mount 17066 15935 5000
Toggle mount 864 887 5000
buttonNative mount 120 129 5000

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🦄 Avatar.Fluent 0.19 0.52 0.37:1 2000 384
🦄 Button.Fluent 0.12 0.22 0.55:1 5000 615
🔧 Checkbox.Fluent 0.66 0.39 1.69:1 1000 662
🎯 Dialog.Fluent 0.17 0.24 0.71:1 5000 828
🔧 Dropdown.Fluent 3.33 0.45 7.4:1 1000 3332
🔧 Icon.Fluent 0.14 0.07 2:1 5000 696
🦄 Image.Fluent 0.09 0.14 0.64:1 5000 465
🔧 Slider.Fluent 1.81 0.5 3.62:1 1000 1807
🔧 Text.Fluent 0.08 0.04 2:1 5000 416
🦄 Tooltip.Fluent 0.16 0.95 0.17:1 5000 803

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
TreeWith60ListItems.default 236 213 1.11:1
IconMinimalPerf.default 767 703 1.09:1
TextMinimalPerf.default 425 395 1.08:1
CarouselMinimalPerf.default 570 532 1.07:1
LayoutMinimalPerf.default 449 420 1.07:1
ListCommonPerf.default 769 727 1.06:1
PopupMinimalPerf.default 805 759 1.06:1
TextAreaMinimalPerf.default 616 579 1.06:1
Image.Fluent 465 440 1.06:1
ChatWithPopoverPerf.default 462 440 1.05:1
DividerMinimalPerf.default 431 411 1.05:1
ReactionMinimalPerf.default 464 444 1.05:1
AlertMinimalPerf.default 339 326 1.04:1
AnimationMinimalPerf.default 485 465 1.04:1
ChatDuplicateMessagesPerf.default 345 333 1.04:1
DialogMinimalPerf.default 838 809 1.04:1
FormMinimalPerf.default 486 468 1.04:1
ImageMinimalPerf.default 467 450 1.04:1
ListNestedPerf.default 659 631 1.04:1
TableMinimalPerf.default 462 445 1.04:1
BoxMinimalPerf.default 426 415 1.03:1
ProviderMergeThemesPerf.default 1770 1726 1.03:1
ProviderMinimalPerf.default 1183 1153 1.03:1
VideoMinimalPerf.default 742 723 1.03:1
Avatar.Fluent 384 374 1.03:1
Dialog.Fluent 828 800 1.03:1
Text.Fluent 416 402 1.03:1
Tooltip.Fluent 803 783 1.03:1
AttachmentSlotsPerf.default 1314 1285 1.02:1
ButtonMinimalPerf.default 213 209 1.02:1
ChatMinimalPerf.default 728 711 1.02:1
DatepickerMinimalPerf.default 54165 52873 1.02:1
DropdownMinimalPerf.default 3346 3283 1.02:1
GridMinimalPerf.default 410 400 1.02:1
ListMinimalPerf.default 602 589 1.02:1
ListWith60ListItems.default 731 716 1.02:1
MenuButtonMinimalPerf.default 1780 1751 1.02:1
Slider.Fluent 1807 1765 1.02:1
ButtonSlotsPerf.default 662 653 1.01:1
ButtonUseCssNestingPerf.default 1223 1205 1.01:1
CheckboxMinimalPerf.default 2984 2967 1.01:1
HeaderMinimalPerf.default 432 426 1.01:1
LoaderMinimalPerf.default 784 776 1.01:1
RosterPerf.default 1290 1283 1.01:1
SliderMinimalPerf.default 1825 1803 1.01:1
SplitButtonMinimalPerf.default 4170 4123 1.01:1
TableManyItemsPerf.default 2192 2164 1.01:1
CustomToolbarPrototype.default 4202 4174 1.01:1
ToolbarMinimalPerf.default 1087 1080 1.01:1
TooltipMinimalPerf.default 1117 1103 1.01:1
DropdownManyItemsPerf.default 796 799 1:1
EmbedMinimalPerf.default 4550 4565 1:1
FlexMinimalPerf.default 337 337 1:1
LabelMinimalPerf.default 448 448 1:1
RadioGroupMinimalPerf.default 514 516 1:1
SegmentMinimalPerf.default 422 424 1:1
SkeletonMinimalPerf.default 422 422 1:1
StatusMinimalPerf.default 802 801 1:1
Checkbox.Fluent 662 660 1:1
AttachmentMinimalPerf.default 199 202 0.99:1
ButtonUseCssPerf.default 929 939 0.99:1
MenuMinimalPerf.default 992 1005 0.99:1
TreeMinimalPerf.default 884 892 0.99:1
Dropdown.Fluent 3332 3350 0.99:1
ButtonOverridesMissPerf.default 1869 1908 0.98:1
HeaderSlotsPerf.default 921 943 0.98:1
InputMinimalPerf.default 1383 1416 0.98:1
ItemLayoutMinimalPerf.default 1412 1438 0.98:1
CardMinimalPerf.default 647 665 0.97:1
RefMinimalPerf.default 255 264 0.97:1
Button.Fluent 615 631 0.97:1
Icon.Fluent 696 719 0.97:1
AccordionMinimalPerf.default 188 199 0.94:1
PortalMinimalPerf.default 179 192 0.93:1
AvatarMinimalPerf.default 219 237 0.92:1

@layershifter layershifter changed the title fix: update RTL in ax() fix: update RTL in ax(), useAx() removal Apr 7, 2021
@@ -8,4 +8,4 @@ export const RTL_PREFIX = 'r';

export const SEQUENCE_PREFIX = '__';

export const DEFINITION_LOOKUP_TABLE: Record<string, MakeStylesMatchedDefinitions> = {};
export const DEFINITION_LOOKUP_TABLE: Record<string, [MakeStylesMatchedDefinitions, boolean /* isRTL */]> = {};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFINITION_LOOKUP_TABLE now knows about definitions' text direction, it works because each key in it is a hash and it's unique for LTR/RTL.

@layershifter layershifter marked this pull request as ready for review April 7, 2021 07:53
…ix/ax-no-context

� Conflicts:
�	packages/make-styles/src/constants.ts
@layershifter layershifter merged commit 45f6f31 into master Apr 7, 2021
@layershifter layershifter deleted the fix/ax-no-context branch April 7, 2021 13:20
@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-tooltip@v9.0.0-alpha.16 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-menu@v9.0.0-alpha.3 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-link@v9.0.0-alpha.20 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-make-styles@v9.0.0-alpha.19 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-divider@v9.0.0-alpha.9 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-image@v9.0.0-alpha.20 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-flex@v9.0.0-alpha.12 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-cards@v1.0.0-beta.79 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-badge@v9.0.0-alpha.22 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-avatar@v9.0.0-alpha.20 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-accordion@v9.0.0-alpha.11 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/react-examples@v8.14.1 has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot
Copy link
Collaborator

🎉@fluentui/make-styles@v9.0.0-alpha.9 has been released which incorporates this pull request.:tada:

Handy links:

miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request May 5, 2021
* fix: update RTL in ax()

* Change files

* fix ax() usage

* cleanup files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants