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

Coachmark - useOffsetHeight should cause re-render on each set state to match v7 functionality #24702

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

micahgodbolt
Copy link
Member

fixes #23545

In v7 (class based) _setHeightOffsetEveryFrame would call setState and update heightOffset regardless of if this value changed or not. This cause componentDidUpdate to fire and for the positioning logic to update the coachmark as the size changed.

In v8 (hooks based), the same logic, ran in useHeightOffset would call setHeightOffset, but if the value did not change, the component would not re-render.

To solve this problem, I'm storing and retrieving the value in an object, which will always be different, and always cause a re-render.

@github-actions github-actions bot added this to the July Project Cycle Q3 2022 milestone Sep 8, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 8, 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 d2d5f89:

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

@size-auditor
Copy link

size-auditor bot commented Sep 8, 2022

Asset size changes

Project Bundle Baseline Size New Size Difference
office-ui-fabric-react fluentui-react-PositioningContainer 68.289 kB 68.311 kB ExceedsBaseline     22 bytes
office-ui-fabric-react fluentui-react-Coachmark 86.519 kB 86.541 kB ExceedsBaseline     22 bytes

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

Baseline commit: 4c31b27421360e80d8435235092a634a67b0418a (build)

@fabricteam
Copy link
Collaborator

📊 Bundle size report

🤖 This report was generated against 4c31b27421360e80d8435235092a634a67b0418a

@fabricteam
Copy link
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 1094 1042 5000
Breadcrumb mount 3186 3052 1000
Checkbox mount 3005 2969 5000
CheckboxBase mount 2609 2634 5000
ChoiceGroup mount 5615 5468 5000
ComboBox mount 1073 1088 1000
CommandBar mount 11964 11502 1000
ContextualMenu mount 13357 13521 1000
DefaultButton mount 1360 1377 5000
DetailsRow mount 4415 4346 5000
DetailsRowFast mount 4490 4373 5000
DetailsRowNoStyles mount 4187 4136 5000
Dialog mount 3432 3367 1000
DocumentCardTitle mount 217 243 1000
Dropdown mount 4151 3830 5000
FocusTrapZone mount 2252 2147 5000
FocusZone mount 1970 1987 5000
IconButton mount 2053 2094 5000
Label mount 413 395 5000
Layer mount 4974 4999 5000
Link mount 540 523 5000
MenuButton mount 1790 1838 5000
MessageBar mount 2436 2387 5000
Nav mount 3757 3767 1000
OverflowSet mount 1229 1234 5000
Panel mount 2798 2770 1000
Persona mount 1087 1116 1000
Pivot mount 1600 1621 1000
PrimaryButton mount 1514 1501 5000
Rating mount 9084 9135 5000
SearchBox mount 1571 1563 5000
Shimmer mount 3485 3305 5000
Slider mount 2208 2199 5000
SpinButton mount 5655 5846 5000
Spinner mount 505 514 5000
SplitButton mount 3619 3568 5000
Stack mount 572 576 5000
StackWithIntrinsicChildren mount 2785 2831 5000
StackWithTextChildren mount 6232 6175 5000
SwatchColorPicker mount 13126 13416 5000
TagPicker mount 3016 3087 5000
TeachingBubble mount 108802 108595 5000
Text mount 502 506 5000
TextField mount 1558 1610 5000
ThemeProvider mount 1416 1361 5000
ThemeProvider virtual-rerender 802 830 5000
ThemeProvider virtual-rerender-with-unmount 2222 2191 5000
Toggle mount 947 966 5000
buttonNative mount 140 154 5000

@micahgodbolt micahgodbolt merged commit df5da17 into microsoft:master Sep 8, 2022
@micahgodbolt micahgodbolt deleted the 23545-coachmarks branch September 8, 2022 17:18
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 9, 2022
* master: (63 commits)
  feat: add helper types to assist DOM element handling (microsoft#24722)
  applying package updates
  Textarea/hc bug (microsoft#24701)
  Fix null ref in use slider (microsoft#24728)
  Add Field unit tests, and remove snapshot tests (microsoft#24706)
  Stress Test: add build commands (microsoft#24575)
  Coachmark - useOffsetHeight should cause re-render on each set state to match v7 functionality (microsoft#24702)
  Implement screener tests for Field components (microsoft#24684)
  Update Field types to clean up react-field.api.md (microsoft#24703)
  fix(Popup): remove rotate(360deg) from PopupContent content styles (microsoft#24432)
  fix(FocusZone): should reset tabindex when focus is outside the zone with prop `shouldResetActiveElementWhenTabFromZone` (microsoft#24463)
  Fix greyed out legend key contrast ratio (microsoft#24714)
  fix: Portal compat should apply `focus-visible` ponyfill (microsoft#24712)
  Fix artifact error (microsoft#24717)
  chore(react-dialog): remove localShorthands in favor of griffel shorthands (microsoft#24715)
  Skip screener checks for draft PRs with exception of appropriately la… (microsoft#24694)
  fix: Remove provider classname from focus styles (microsoft#24710)
  feat: autocontrolled `useTable` hook (microsoft#24688)
  feat: add dialog properties to getNativeElementProps (microsoft#24698)
  Using migrate rather than upgrade term (microsoft#24695)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Coachmark doesn't render teaching bubble in the correct place when not in the right
6 participants