Storybook: Move debug UI mobile/RTL toggles to the toolbar - #4241
Conversation
npm SnapshotWant to try this PR's changes before it merges? Comment |
|
Size Change: 0 B Total Size: 499 kB ℹ️ View Unchanged
|
| "Treat the host as a phone or tablet (Perseus isMobile). Pair " + | ||
| "with a phone Viewport for the full experience.", |
There was a problem hiding this comment.
Pair with a phone Viewport for the full experience.
No action needed: I wonder if we could do this automagically in the future by rewiring some Storybook stuff 🤔
There was a problem hiding this comment.
👍 I thought about this too. It was tricky to do right as each toggled back and forth, so instead of making it more complicated, I opted for simplicity to start. We can adjust this further or "tie" the two settings together in a future PR.
Also, in the future as we refactor layout to be responsive based on container sizing and untangle our "we do this special thing on mobile" (for no good reason), I think we'll have less and less need for this special "isMobile" toggle).
|
|
||
| // The toolbar's "story default" leaves per-story apiOptions in charge so | ||
| // that mobile stories stay mobile. | ||
| const isMobile = mobileOverride ?? apiOptions.isMobile ?? false; |
There was a problem hiding this comment.
I'm not sure what to do about this, but it seems like this could be a little bit confusing. Like what if you wanted to turn off isMobile for a story that had it enabled in apiOptions.
Again, not sure if there's anything worth doing right now though. We can see if it becomes an issue in the future (I'm guessing it won't be).
There was a problem hiding this comment.
Agree!
Right now, it looks like we use this primarily to build a Mobile-specific version of a story. I think now that this has become a Storybook setting, it might be possible to provide these two stories somewhat automatically.
| onToggleMobile, | ||
| onToggleRtl, | ||
| }: DebugHeaderProps): React.ReactElement => { | ||
| export const DebugHeader = ({title}: DebugHeaderProps): React.ReactElement => { |
There was a problem hiding this comment.
Do we even need a DebugHeader if it's basically not doing anything anymore? Is it even a "debug header" at this point?
There was a problem hiding this comment.
You're right. We have a title in this component, but the only thing we ever pass to it is Widget and "". I've deleted the file and cleaned up the usages.
| onToggleMobile={toggleMobile} | ||
| /> | ||
| {/* Title */} | ||
| <DebugHeader title={title} /> |
There was a problem hiding this comment.
Yeah, I dunno. Doesn't seem to warrant being its own component.
| @@ -0,0 +1,16 @@ | |||
| import * as React from "react"; | |||
|
|
|||
| type StorybookViewOptions = { | |||
There was a problem hiding this comment.
What if we wanted to follow this pattern for other Storybook options. Would we need to make a separate context? Or could this just be StorybookOptions and StorybookOptionsContext?
There was a problem hiding this comment.
That's a good callout! We already have another Perseus-specific options context in StorybookFeatureFlagsContext.
I wonder if it's better to break contexts apart to specific concerns or to group them?
I'm going to land this PR, but I can open another PR to do a bit of unifying of these.
896bf75 to
00a84f1
Compare
2 participants
Summary:
Today, our widget stories, use a mix of
ServerItemRendererWithDebugUIandQuestionRendererForStories. In #4217 I attempted to unify the component we use for all widget stories, but it hits Storybookplaytest failures. This is because theServerItemRendererWithDebugUIhas two switches in the header which are the first things that receive focus plus they addswitchrole elements to the DOM that confuse selection of elements.So, this PR moves these two debug header items to be true Storybook toolbar toggles. This removes them from the Storybook canvas and will eliminate the confounding elements for #4217.
Note that the Mobile toggle may not be what you think. It controls the
isMobileflag that we pass toapiOptions. It's not a statement of viewport width (use the built-in Viewport Storybook toolbar item for that). I've chosen not to entwine these because you can have a wide viewport (tablet) and still haveisMobileenabled and (unfortunately) you can have a narrow viewport (such as when Perseus is embedded in a narrow panel) and still haveisMobile: false.Issue: LEMS-4611
Test plan:
pnpm startNavigate to the Dropdown widget story. Change the mobile toggle and test. Chagne the Viewport widgth and test.