Skip to content

Observe Combobox trigger for resize#1333

Merged
devongovett merged 7 commits into
mainfrom
combobox-resize-observer
Dec 7, 2020
Merged

Observe Combobox trigger for resize#1333
devongovett merged 7 commits into
mainfrom
combobox-resize-observer

Conversation

@snowystinger
Copy link
Copy Markdown
Member

combobox menu should remeasure whenever the combobox input/button change size

Closes

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

// Measure the width of the button to inform the width of the menu (below).
let [buttonWidth, setButtonWidth] = useState(null);
let {scale} = useProvider();
useLayoutEffect(() => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does resize observer always get called on the initial render? I think at least when not supported it wouldn't based on my reading of the code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

O good point, forgot about the unsupported case. Otherwise yes, it seems to be called on the initial render https://drafts.csswg.org/resize-observer/#ref-for-element%E2%91%A3

ResizeObserver’s notifications can be used to respond to changes in Element's size. Some interesting facts about these observations:

Observation will fire when watched Element is inserted/removed from DOM.

Observation will fire when watched Element display gets set to none.

Observations do not fire for non-replaced inline Elements.

Observations will not be triggered by CSS transforms.

Observation will fire when observation starts if Element is being rendered, and Element’s size is not 0,0.

Copy link
Copy Markdown
Member Author

@snowystinger snowystinger Dec 2, 2020

Choose a reason for hiding this comment

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

It might be better to implement that in useResizeObserver's fallback case. Trigger a "resize" event on mount? Ah, but then scale changes...

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Comment on lines +117 to +121
useLayoutEffect(() => {
let buttonWidth = buttonRef.current.UNSAFE_getDOMNode().offsetWidth;
let buttonWidth = unwrappedButtonRef.current.offsetWidth;
let inputWidth = inputRef.current.offsetWidth;
setMenuWidth(buttonWidth + inputWidth);
}, [scale, buttonRef, inputRef]);
}, [scale, unwrappedButtonRef, inputRef]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would this work?

useLayoutEffect(onResize, [scale, onResize])

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Copy link
Copy Markdown
Collaborator

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

Should there be ComboBox and Picker resize tests? And should they resize multiple times?

@snowystinger
Copy link
Copy Markdown
Member Author

doing resize tests is quite difficult, there are two ways to do it:

  1. mock the prototype, but this is brittle, should anything else also check sizes or if the order changes a little
  2. mock the instance specific attributes, which requires rendering first, then getting the instance, then mocking, which is unfortunately not how most of our elements with measuring do things

We have very few of these tests because they are so hard and brittle, as such I opted to leave them out of this one, the behavior is fairly easy to observer in the stories.

@snowystinger snowystinger mentioned this pull request Dec 7, 2020
5 tasks
@devongovett devongovett merged commit 0b42aed into main Dec 7, 2020
@devongovett devongovett deleted the combobox-resize-observer branch December 7, 2020 23:10
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