Skip to content

fix: new overflow items should only be enqueued while observing#25122

Merged
ling1726 merged 7 commits intomicrosoft:masterfrom
ling1726:chore/react-overflow-tests
Oct 7, 2022
Merged

fix: new overflow items should only be enqueued while observing#25122
ling1726 merged 7 commits intomicrosoft:masterfrom
ling1726:chore/react-overflow-tests

Conversation

@ling1726
Copy link
Contributor

@ling1726 ling1726 commented Oct 6, 2022

Current Behavior

Currently overflow items are enqueued immediately when added to the manager. This can create subtle bugs because all options are set during .observe(). Some of those options can alter the priority of the items in the queue.

New Behavior

Adds a new observing flag to the overflow manager and enqueues all overflow items during observe() any new items added while observing is true will be enqueued directly.

Also adds tests

Related Issue(s)

Fixes #

Currently overflow items are enqueued immediately when added to the
manager. This can create subtle bugs because all options are set during
`.observe()`. Some of those options can alter the priority of the items
in the queue.

Adds a new `observing` flag to the overflow manager and enqueues all
overflow items during `observe()` any new items added while `observing`
is true will be enqueued directly.

Also adds tests
@ling1726 ling1726 marked this pull request as ready for review October 6, 2022 17:38
@ling1726 ling1726 requested a review from a team as a code owner October 6, 2022 17:38
@fabricteam
Copy link
Collaborator

fabricteam commented Oct 6, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
priority-overflow
createOverflowManager
2.936 kB
1.212 kB
3.001 kB
1.239 kB
65 B
27 B
react-overflow
hooks only
10.685 kB
4.104 kB
10.75 kB
4.125 kB
65 B
21 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
62.781 kB
17.574 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
189.227 kB
52.474 kB
react-components
react-components: FluentProvider & webLightTheme
33.4 kB
11.008 kB
react-portal-compat
PortalCompatProvider
5.857 kB
1.978 kB
🤖 This report was generated against c917857fe0ff881c1b1ba24778f51f5939b08ac1

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 6, 2022

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 550938c:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Oct 6, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: c917857fe0ff881c1b1ba24778f51f5939b08ac1 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 6, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1692 1667 5000
Button mount 1158 1175 5000
FluentProvider mount 1911 1929 5000
FluentProviderWithTheme mount 720 731 10
FluentProviderWithTheme virtual-rerender 679 676 10
FluentProviderWithTheme virtual-rerender-with-unmount 729 729 10
MakeStyles mount 2307 2253 50000
SpinButton mount 3246 3140 5000

@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "new overflow items should only be enqueued while observing",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"comment": "new overflow items should only be enqueued while observing",
"comment": "fix: new overflow items should only be enqueued while observing",

@ling1726 ling1726 enabled auto-merge (squash) October 7, 2022 08:44
@ling1726 ling1726 merged commit d596836 into microsoft:master Oct 7, 2022
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Oct 10, 2022
* master: (23 commits)
  Revert "chore: screener-run workflow should report to PR (microsoft#25144)" (microsoft#25145)
  chore: screener-run workflow should report to PR (microsoft#25144)
  applying package updates
  fix: The Tooltip should not hide if it gets keyboard focus (microsoft#25138)
  fix: Tooltip should not hide if an element inside it gets focused (microsoft#25140)
  Create react-migration-v8-v9 with shims and stories (microsoft#25121)
  fix: CheckboxField to set a generated ID on the input, to match the label's htmlFor (microsoft#25079)
  feat: Overflow menu should be registered in overflowManager (microsoft#25091)
  fix: version-bump generator removes beachball disallowedChangeType config (microsoft#25130)
  fix: new overflow items should only be enqueued while observing (microsoft#25122)
  fix(script): allow runPublished call from CLI (microsoft#25127)
  feat: add vertical variation for toolbar (microsoft#24940)
  ProgressField implementation and stories (microsoft#25103)
  fix: Dropdown icon layout with no placeholder/value (microsoft#25049)
  chore: add a bundle size fixture (Button, Provider & theme) (microsoft#25113)
  feat: Adding subtle transition between states on Button components (microsoft#25106)
  Revert "chore: screener-run workflow should report to PR (microsoft#25096)" (microsoft#25115)
  chore: screener-run workflow should report to PR (microsoft#25096)
  fix(react-dialog): aria-* properties should be reassignable (microsoft#25092)
  fix(scripts): don't run publish if web-components are affected (microsoft#25095)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
…osoft#25122)

* fix: new overflow items should only be enqueued while observing

Currently overflow items are enqueued immediately when added to the
manager. This can create subtle bugs because all options are set during
`.observe()`. Some of those options can alter the priority of the items
in the queue.

Adds a new `observing` flag to the overflow manager and enqueues all
overflow items during `observe()` any new items added while `observing`
is true will be enqueued directly.

Also adds tests

* revert tabster changes

* changefiles

* just enqueue

* add unit tests

* set observing to false
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.

4 participants