Storybook: Standardize stories on ServerItemRendererWithDebugUI - #4217
jeremywiebe wants to merge 8 commits into
Conversation
npm SnapshotWant to try this PR's changes before it merges? Comment |
jeremywiebe
changed the title
|
Size Change: +11 B (0%) Total Size: 499 kB 📦 View Changed
ℹ️ View Unchanged
|
|
|
||
| export const Answerless: Story = { | ||
| args: { | ||
| item: getFullGroupTestItem(), |
There was a problem hiding this comment.
Not so answerless after all. 😂
|
|
||
| export const Answerless: Story = { | ||
| args: { | ||
| item: getFullGroupTestItem(), |
| args: { | ||
| item: generateTestPerseusItem({question: basicBlankQuestion}), | ||
| }, | ||
| args: {question: basicBlankQuestion}, |
There was a problem hiding this comment.
I'm curious about the motivation here as the ticket and PR description didn't make it totally clear to me.
My gut reaction is that I personally would prefer us using an actual PerseusItem since like probably 90-99% of real world use of Perseus uses a PerseusItem.
There was a problem hiding this comment.
I ran across this while doing some unrelated work on Chromatic... The motivation is that we are inconsistent in how we build stories for widgets. I much prefer using the smaller data structure and narrowed Renderer in these tests because we don't use any of the features that the ServerItemRenderer (SIR) provides. Although externally most of the world uses SIR, I don't think that's a great reason to use it here. Even if we eventually unify the external API to use SIR everywhere, I don't see us getting rid of Renderer.
That said, I could change this PR to unify usages in the opposite direction (use SIR for all widget stories) if you feel strongly. :)
2fef37f to
6dbd000
Compare
jeremywiebe
changed the title
d5f2883 to
bec3e94
Compare
jeremywiebe
changed the title
| options: generateCategorizerOptions({ | ||
| ...args, | ||
| <ServerItemRendererWithDebugUI | ||
| item={generateTestPerseusItem({ |
There was a problem hiding this comment.
I personally like separating out JSX from regular JS logic when the JS logic starts getting complex.
) => {
const item = generateTestPerseusRenderer({
// ...etc...
})
return (
<ServerItemRendererWithDebugUI
item={item}Same note for a lot of these.
There was a problem hiding this comment.
Don't know why but it doesn't bother me much...
I've refactored them to generate the item outside of the JSX elements.
| alt: "Fresco painting", | ||
| longDescription: "long description", | ||
| <ServerItemRendererWithDebugUI | ||
| item={generateTestPerseusItem({ |
There was a problem hiding this comment.
OMG especially these. This is my nightmare fuel seeing all these item generation logic in JSX.
| export const AnswerlessTable: Story = { | ||
| args: { | ||
| item: tableItem, | ||
| item: answerlessTableItem, |
There was a problem hiding this comment.
Womp womp 2: the wompening
896bf75 to
00a84f1
Compare
…ndererWithDebugUI
## Summary: Today, our widget stories, use a mix of `ServerItemRendererWithDebugUI` and `QuestionRendererForStories`. In #4217 I attempted to unify the component we use for all widget stories, but it hits Storybook `play` test failures. This is because the `ServerItemRendererWithDebugUI` has two switches in the header which are the first things that receive focus plus they add `switch` role 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. <img width="311" height="172" alt="image" src="https://github.com/user-attachments/assets/007e525a-2f4d-49b8-920d-3a92e51575c3" /> Note that the Mobile toggle may not be what you think. It controls the `isMobile` flag that we pass to `apiOptions`. 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 have `isMobile` enabled and (unfortunately) you can have a narrow viewport (such as when Perseus is embedded in a narrow panel) and still have `isMobile: false`. Issue: LEMS-4611 ## Test plan: `pnpm start` Navigate to the [Dropdown](http://localhost:6006/?path=/story/widgets-dropdown-visual-regression-tests-initial-state--basic-dropdown) widget story. Change the mobile toggle and test. Chagne the Viewport widgth and test. Author: jeremywiebe Reviewers: handeyeco, jeremywiebe, benchristel, nishasy, mark-fitzgerald, ivyolamit, catandthemachines Required Reviewers: Approved By: handeyeco Checks: ⏭️ 2 checks have been skipped, ✅ 10 checks were successful Pull Request URL: #4241
118ee5f to
2f9f6ee
Compare
2 participants
This PR adjusts stories to render widget stories with
ServerItemRendererWithDebugUIinstead of using smallerPerseusRenderers. It's mostly a bit of standardizing (ideally, I think having all our widgets approach building stories in the same, consistent way unless there's a real need to diverge).This also preserves distinct answerful and answerless group and table stories.
Issue: LEMS-4592
Test plan
pnpm typecheckpnpm lintpnpm test