Popover: rewrite Storybook examples using controls #42903
Merged
Conversation
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
ciampo
commented
Aug 2, 2022
ciampo
commented
Aug 2, 2022
| import { Provider as SlotFillProvider } from '../../slot-fill'; | ||
| import Popover from '../'; | ||
|
|
||
| // Format: "[yAxis] [xAxis]" |
Contributor
Author
There was a problem hiding this comment.
It actually looks like the position prop accepted also a third value, called corner — although I couldn't find any reference in the old popover documentation and I didn't have enough time to investigate deeper
ciampo
commented
Aug 2, 2022
Comment on lines
+76
to
+78
| left: 1px; | ||
| height: 2px; | ||
| width: 100%; | ||
| right: 1px; |
Contributor
Author
There was a problem hiding this comment.
Happy to move this adjustment to a separate PR if necessary
ciampo
added
[Package] Components
67 tasks
|
Size Change: +10 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Aug 3, 2022
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
A much needed improvement for the intense Popover work going on. Thanks!
| }, | ||
| parameters: { | ||
| knobs: { disable: false }, | ||
| docs: { source: { state: 'open' } }, |
Member
There was a problem hiding this comment.
I think we can keep the code snippets collapsed for these stories, since they're not really useful in the current state. (Maybe with the exception of the iframe one?)
Contributor
Author
There was a problem hiding this comment.
I removed the whole parameters key for now in 0381cd8. I'm sure we'll iterate over this file soon enough, especially when we'll convert this component to TypeScript
ciampo
force-pushed
the
refactor/popover-storybook-controls
branch
2 times, most recently
from
0381cd8 to
a72c13e
Compare
ciampo
force-pushed
the
refactor/popover-storybook-controls
branch
from
a72c13e to
3612384
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
In the context of #42770, the Storybook examples are being of little help when working on the
Popovercomponent in isolation.This PR refactors the Storybook examples to use controls (instead of the deprecated knobs add-on), and also rewrites some of the stories in a way that hopefully showcases the usages of the component in a better way compared to how it was before.
Why?
Better Storybook examples = better dev experience (both as a component author, and as a component consumer).
How?
Popoverpropsdefaultexample has been changed to mirror the example snippet in the component's README (see notes below). I feel like the previous default example was not really useful anyway.positionsexample has been replaced with a new story that showcases all possible values forplacement(ie. the new prop that is supposed to replaceposition)Testing Instructions
Known issues
Working on this refactor highlighted a couple of issues:
The
Popoveris not anchoring itself correctly to its parent (which is the default behaviour when an explicit anchor it not passed):Kapture.2022-08-02.at.19.31.16.mp4
This behavior can be also spotted on
trunkby visiting the positioning storybook example, and noting that thePopoveris not positioning itself correctly with respect to its parent (the pink bordered box). This is likely another regression introduced by #40740, and if folks confirm it, I will create a new issue and add it to #42770The positioning of the
Popoveris a bit off in theiframeexampleKapture.2022-08-02.at.19.53.01.mp4
I haven't had time to debug this example in depth yet, but it looks like the offset applied to the
Popoverto compensate theiFramepositioning is not correct. It's interesting (and worrying) that thePopoverdoesn't work as expected with a simpleiframeexample. We should understand if something is wrong in the way I set up the Storybook example, or if this is another bug that we need to add to #42770 and fix.