-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Moving OverflowSet to @fluentui/react-internal package #15389
Moving OverflowSet to @fluentui/react-internal package #15389
Conversation
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 a1e715c:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 1e626b101db22df0f12bf04f42be6e94c1c5fdcb (build) |
Looks like this will require some updates in the charting package to use new props. |
…eat/react-internal-overflowset # Conflicts: # packages/react-next/etc/react-next.api.md
…eat/react-internal-overflowset
@@ -827,10 +824,11 @@ exports[`Component Examples renders CommandBar.Basic.Example.tsx correctly 1`] = | |||
</div> | |||
<div | |||
className= | |||
ms-OverflowSet-overflowButton | |||
ms-CommandBar-primaryCommand |
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.
Do you know why this class name change happened?
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.
Thanks for catching this! getNativeProps was placed incorrectly in the wrapper div and was not applying the className properly.
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.
Nice to see the snapshot tests being useful! :)
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.
Hmm original question still stands though--why did the class name change (even after the update to native props location)?
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.
I updated the pr and it should be good now. The snapshot change was due to ButtonGrid
applying {...props} after the classNames and then applying and overwriting them.
The remaining snapshot changes for VerticalBarChart
,AreaChart
, OverflowSet
, and ReizeGroup
are due to the FocusZone
removal from OverflowSet
and id updates:
…eat/react-internal-overflowset
…ct-internal snapshots
@@ -197,11 +197,6 @@ export class AreaChartMultipleExample extends React.Component<{}, IAreaChartBasi | |||
legendsOverflowText={'Overflow Items'} | |||
yAxisTickFormat={d3.format('$,')} | |||
legendProps={{ |
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.
Just to confirm, did you check out these examples to verify that they still work as before (even without the FocusZone)? You can compare with the PR deploy site of charting in master. http://fabricweb.z5.web.core.windows.net/pr-deploy-site/refs/heads/master/
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.
I don't see any behavior changes in the Charting components after the FocusZone
removal.
…eat/react-internal-overflowset
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.
Thanks for all the work on this! I'm going to push an update removing the scss file and then merge it.
@@ -1,4 +1,4 @@ | |||
@import '../../common/common'; | |||
@import '~@fluentui/common-styles/dist/sass/common'; |
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.
I think we can delete this file--I don't see any references to it
Pull request checklist
$ yarn change
Description of changes
OverflowSet
from react-next to react-internal (including function component conversion from Updating OverflowSet to a function component inside React-Next #13077).OverflowSet
in react-internal.OverflowSetNext
.OverflowSet
.