feat: add portal compat implementation to v8 & v0#22541
Merged
layershifter merged 8 commits intoMay 12, 2022
Conversation
layershifter
force-pushed
the
feat/portal-compat-in-v0-n-v8
branch
from
af75576 to
8f5db9c
Compare
layershifter
force-pushed
the
feat/portal-compat-in-v0-n-v8
branch
from
8f5db9c to
5cb911e
Compare
|
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 2a55e6c:
|
Collaborator
📊 Bundle size report🤖 This report was generated against 5ab96eaa4a341c54b1e6e609f4e518c57cd1320b |
Asset size changes
Baseline commit: 5ab96eaa4a341c54b1e6e609f4e518c57cd1320b (build) |
layershifter
force-pushed
the
feat/portal-compat-in-v0-n-v8
branch
from
5cb911e to
6fd48c5
Compare
layershifter
force-pushed
the
feat/portal-compat-in-v0-n-v8
branch
from
6fd48c5 to
5b7a811
Compare
Collaborator
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 168 | 141 | 1.19:1 |
| FormMinimalPerf.default | 425 | 391 | 1.09:1 |
| RefMinimalPerf.default | 250 | 234 | 1.07:1 |
| AvatarMinimalPerf.default | 201 | 189 | 1.06:1 |
| CheckboxMinimalPerf.default | 2814 | 2663 | 1.06:1 |
| ProviderMinimalPerf.default | 405 | 381 | 1.06:1 |
| GridMinimalPerf.default | 345 | 329 | 1.05:1 |
| HeaderMinimalPerf.default | 373 | 354 | 1.05:1 |
| HeaderSlotsPerf.default | 790 | 751 | 1.05:1 |
| TextAreaMinimalPerf.default | 486 | 462 | 1.05:1 |
| ButtonMinimalPerf.default | 160 | 154 | 1.04:1 |
| SkeletonMinimalPerf.default | 340 | 327 | 1.04:1 |
| StatusMinimalPerf.default | 677 | 654 | 1.04:1 |
| AnimationMinimalPerf.default | 543 | 525 | 1.03:1 |
| ButtonOverridesMissPerf.default | 1533 | 1492 | 1.03:1 |
| ImageMinimalPerf.default | 365 | 355 | 1.03:1 |
| ItemLayoutMinimalPerf.default | 1187 | 1155 | 1.03:1 |
| LabelMinimalPerf.default | 384 | 372 | 1.03:1 |
| MenuButtonMinimalPerf.default | 1773 | 1722 | 1.03:1 |
| DialogMinimalPerf.default | 772 | 760 | 1.02:1 |
| DropdownMinimalPerf.default | 3069 | 3018 | 1.02:1 |
| EmbedMinimalPerf.default | 4201 | 4135 | 1.02:1 |
| InputMinimalPerf.default | 1301 | 1273 | 1.02:1 |
| ListMinimalPerf.default | 505 | 497 | 1.02:1 |
| ListNestedPerf.default | 542 | 529 | 1.02:1 |
| ListWith60ListItems.default | 653 | 643 | 1.02:1 |
| SegmentMinimalPerf.default | 343 | 335 | 1.02:1 |
| TableManyItemsPerf.default | 1955 | 1917 | 1.02:1 |
| ToolbarMinimalPerf.default | 968 | 948 | 1.02:1 |
| TreeMinimalPerf.default | 836 | 818 | 1.02:1 |
| CardMinimalPerf.default | 572 | 564 | 1.01:1 |
| ChatDuplicateMessagesPerf.default | 295 | 292 | 1.01:1 |
| ChatMinimalPerf.default | 753 | 742 | 1.01:1 |
| FlexMinimalPerf.default | 286 | 284 | 1.01:1 |
| PopupMinimalPerf.default | 634 | 630 | 1.01:1 |
| SplitButtonMinimalPerf.default | 4409 | 4381 | 1.01:1 |
| TableMinimalPerf.default | 388 | 386 | 1.01:1 |
| TextMinimalPerf.default | 335 | 331 | 1.01:1 |
| CustomToolbarPrototype.default | 2756 | 2723 | 1.01:1 |
| TooltipMinimalPerf.default | 1111 | 1102 | 1.01:1 |
| LoaderMinimalPerf.default | 689 | 691 | 1:1 |
| ProviderMergeThemesPerf.default | 1263 | 1269 | 1:1 |
| RadioGroupMinimalPerf.default | 449 | 451 | 1:1 |
| SliderMinimalPerf.default | 1687 | 1688 | 1:1 |
| VideoMinimalPerf.default | 659 | 658 | 1:1 |
| AttachmentSlotsPerf.default | 1057 | 1073 | 0.99:1 |
| CarouselMinimalPerf.default | 482 | 489 | 0.99:1 |
| MenuMinimalPerf.default | 861 | 869 | 0.99:1 |
| ReactionMinimalPerf.default | 381 | 384 | 0.99:1 |
| ChatWithPopoverPerf.default | 378 | 386 | 0.98:1 |
| DividerMinimalPerf.default | 346 | 353 | 0.98:1 |
| DropdownManyItemsPerf.default | 665 | 677 | 0.98:1 |
| ListCommonPerf.default | 648 | 658 | 0.98:1 |
| PortalMinimalPerf.default | 168 | 172 | 0.98:1 |
| IconMinimalPerf.default | 590 | 605 | 0.98:1 |
| DatepickerMinimalPerf.default | 5816 | 5969 | 0.97:1 |
| BoxMinimalPerf.default | 327 | 340 | 0.96:1 |
| ButtonSlotsPerf.default | 536 | 561 | 0.96:1 |
| LayoutMinimalPerf.default | 342 | 360 | 0.95:1 |
| RosterPerf.default | 1090 | 1148 | 0.95:1 |
| AccordionMinimalPerf.default | 138 | 147 | 0.94:1 |
| AlertMinimalPerf.default | 271 | 287 | 0.94:1 |
| TreeWith60ListItems.default | 173 | 185 | 0.94:1 |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 932 | 925 | 5000 | |
| Breadcrumb | mount | 2824 | 2805 | 1000 | |
| Checkbox | mount | 1628 | 1542 | 5000 | |
| CheckboxBase | mount | 1353 | 1496 | 5000 | |
| ChoiceGroup | mount | 4910 | 4982 | 5000 | |
| ComboBox | mount | 1041 | 1081 | 1000 | |
| CommandBar | mount | 10875 | 11083 | 1000 | |
| ContextualMenu | mount | 12261 | 12302 | 1000 | |
| DefaultButton | mount | 1175 | 1172 | 5000 | |
| DetailsRow | mount | 4074 | 4115 | 5000 | |
| DetailsRowFast | mount | 4320 | 4077 | 5000 | |
| DetailsRowNoStyles | mount | 3819 | 3804 | 5000 | |
| Dialog | mount | 2461 | 2466 | 1000 | |
| DocumentCardTitle | mount | 197 | 202 | 1000 | |
| Dropdown | mount | 3605 | 3658 | 5000 | |
| FocusTrapZone | mount | 1928 | 2062 | 5000 | |
| FocusZone | mount | 1984 | 1868 | 5000 | |
| IconButton | mount | 1766 | 1970 | 5000 | |
| Label | mount | 391 | 416 | 5000 | |
| Layer | mount | 3159 | 3167 | 5000 | |
| Link | mount | 527 | 515 | 5000 | |
| MenuButton | mount | 1578 | 1509 | 5000 | |
| MessageBar | mount | 2289 | 2372 | 5000 | |
| Nav | mount | 3507 | 3455 | 1000 | |
| OverflowSet | mount | 1107 | 1134 | 5000 | |
| Panel | mount | 2327 | 2241 | 1000 | |
| Persona | mount | 1058 | 1033 | 1000 | |
| Pivot | mount | 1479 | 1449 | 1000 | |
| PrimaryButton | mount | 1330 | 1309 | 5000 | |
| Rating | mount | 7971 | 7858 | 5000 | |
| SearchBox | mount | 1408 | 1273 | 5000 | |
| Shimmer | mount | 2724 | 2532 | 5000 | |
| Slider | mount | 1965 | 2035 | 5000 | |
| SpinButton | mount | 5580 | 5287 | 5000 | |
| Spinner | mount | 486 | 488 | 5000 | |
| SplitButton | mount | 3298 | 3231 | 5000 | |
| Stack | mount | 526 | 522 | 5000 | |
| StackWithIntrinsicChildren | mount | 2669 | 2499 | 5000 | |
| StackWithTextChildren | mount | 5749 | 5548 | 5000 | |
| SwatchColorPicker | mount | 11863 | 11913 | 5000 | |
| TagPicker | mount | 2837 | 2905 | 5000 | |
| TeachingBubble | mount | 97750 | 97272 | 5000 | |
| Text | mount | 424 | 417 | 5000 | |
| TextField | mount | 1407 | 1469 | 5000 | |
| ThemeProvider | mount | 1265 | 1199 | 5000 | |
| ThemeProvider | virtual-rerender | 720 | 666 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1976 | 2040 | 5000 | |
| Toggle | mount | 832 | 837 | 5000 | |
| buttonNative | mount | 149 | 143 | 5000 |
layershifter
commented
May 2, 2022
| logger('STEP 2. Add Fluent UI dependency to test project..'); | ||
|
|
||
| const packedPackages = await packProjectPackages(logger, config.paths.packages(), ['@fluentui/react-northstar']); | ||
| const packedPackages = await packProjectPackages(logger, config.paths.base(), ['@fluentui/react-northstar']); |
Member
Author
There was a problem hiding this comment.
As we have a package outside packages/fluentui I had to change the root to include @fluentui/react-portal-compat-context into the list of packages to build.
layershifter
commented
May 2, 2022
| "@fluentui/react-component-ref": "^0.62.0", | ||
| "@fluentui/react-icons-northstar": "^0.62.0", | ||
| "@fluentui/react-northstar-styles-renderer": "^0.62.0", | ||
| "@fluentui/react-portal-compat-context": "^9.0.0-rc.0", |
Member
Author
There was a problem hiding this comment.
For v9 we don't use carets, but as it goes to production versions of libraries I think that it's better to actually use them.
Contributor
There was a problem hiding this comment.
It makes sense, but there is a risk of API breakage before release
Member
Author
There was a problem hiding this comment.
To clarify, this is only for @fluentui/react-portal-compat-context. If there will be a breaking change I will major bump the package instead, but it's unlikely will ever happen.
layershifter
commented
May 2, 2022
Comment on lines
+28
to
+31
| const classes = className.split(' ').filter(Boolean); | ||
|
|
||
| if (element) { | ||
| element.setAttribute('class', className); | ||
| element.classList.add(...classes); |
Member
Author
There was a problem hiding this comment.
Had to rework this section because otherwise .setAttribute() overwrites all classes and caused glitches during theme switches.
ling1726
reviewed
May 3, 2022
Comment on lines
+40
to
+45
| return () => { | ||
| if (element) { | ||
| element.classList.remove(...classes); | ||
| element.removeAttribute('dir'); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Previous behaviour didn't do this, are you just adding this now for correctness ?
Member
Author
There was a problem hiding this comment.
Adding now to be more consistent.
ling1726
approved these changes
May 3, 2022
miroslavstastny
approved these changes
May 3, 2022
Hotell
reviewed
May 6, 2022
| @@ -1,3 +1,6 @@ | |||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |||
| // @ts-ignore "react-portal-compat-context" uses v9 configs via path aliases | |||
Contributor
There was a problem hiding this comment.
wondering if we need this ? react-portal-compat-context is a dependency of this package so to be able to get proper types etc one needs to run lage build --to which will trigger build also for this v9 package, thus I assume this can be removed ?
Member
Author
There was a problem hiding this comment.
Yeah, you're totally correct 🐱 I made a change for N*, but then totally forgot about "build-me-first" behavior in @fluentui/react...
Hotell
approved these changes
May 6, 2022
Member
Author
|
@microsoft/cxe-red can someone please check this PR? |
spmonahan
approved these changes
May 10, 2022
…eat/portal-compat-in-v0-n-v8 � Conflicts: � packages/react-components/react-portal-compat/package.json
marwan38
pushed a commit
to marwan38/fluentui
that referenced
this pull request
Jun 13, 2022
* feat: add portal compat implementation to v8 & v0 * fix config * fix project test * remove useless tsignore * fix dependency versions * add changelog entry * fix path
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Behavior
This PR adds implementation of portal compatibility to
@fluentui/react&@fluentui/react-northstarbased on RFC #21774.Technically, these changes could go in separate PRs, but I wanted to make visible for reviewers changes in both libraries. The basic idea is to consume a callback from
usePortalCompat()and call it when an element that is used inReactDOM.createPortal()is created.The compat implemented can be found in #22510, this PR adds
@fluentui/react-portal-compat-contextto dependencies of both packages.👀 Check demo to see how it works.
Related Issue(s)
Follows #22466, #22510.