Fix: MenuButton accepts falsy children #1934
Conversation
Merge v0.24.0 into develop
Initialize Tokens 2.0
chore(odyssey-react-mui): translations for odyssey-react-mui.properties
…theme fix: use odyssey tokens inside mui components style override
Rebrand: Link
refactor(odyssey-react-mui): migrate Circular Progress to design tokens
fix: added incremental to root TSconfig
…replace all touched tokens
fix: memoized the Link component
chrispulsinelli-okta
requested review from
ganeshsomasundaram-okta,
KevinGhadyani-Okta,
kamronbatmanghelich-okta and
a team
as code owners
fix(odyssey-react-mui): make Autocomplete option styles match Select
…-MenuItem fix: add back isDisabled to MenuItem
…-MenuItem fix: remove useless isDestructive prop
chrispulsinelli-okta
force-pushed
the
menu-button-accept-falsy-children
branch
from
8ee066f
to
ce37e88
Compare
Merge release/v1.0.2 into develop
| @@ -91,7 +91,7 @@ const storybookMeta: Meta<MenuButtonProps> = { | |||
| description: "The <MenuItem> components within the Menu", | |||
| table: { | |||
| type: { | |||
| summary: "[MenuItem | Divider | ListSubheader]", | |||
| summary: "[MenuItem | Divider | ListSubheader | NullElement]", | |||
There was a problem hiding this comment.
Isn't this documentation only?
Does it need to be in the props table?
There was a problem hiding this comment.
I'm not quite sure what you're asking here. Can you clarify?
There was a problem hiding this comment.
Props table is just a string. We don't need to put TypeScript types in here in our docs; TS handles that for you in the component.
There was a problem hiding this comment.
Maybe I'm missing something, but this is supposed to reflect the children props of MenuButton, no?
To clarify, are you asking for this to be reverted too?
| /** | ||
| * Note that the falsy children in the following MenuButton won't appear in the source | ||
| * code in the actual story in your browser, but this illustrates that there are no | ||
| * compile-time issues with the falsy items passed as children. | ||
| */ | ||
| export const FalsyChildren: StoryObj<MenuButtonProps> = { | ||
| args: { | ||
| buttonLabel: "More actions", | ||
| children: [ | ||
| <MenuItem key="1">Truthy Menu Item</MenuItem>, | ||
| false, | ||
| null, | ||
| undefined, | ||
| ], | ||
| }, | ||
| play: async ({ | ||
| args, | ||
| canvasElement, | ||
| step, | ||
| }: { | ||
| args: MenuButtonProps; | ||
| canvasElement: HTMLElement; | ||
| step: PlaywrightProps<MenuButtonProps>["step"]; | ||
| }) => { | ||
| clickMenuButton({ canvasElement, step })(args, "Menu Button Simple"); | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
I don't think we want a story for this. Seems like a unit test is enough, but we're unit testing our types right? It always accepted falsy children just that TS complained.
We shouldn't be adding unit tests for type checks. We should expect TS works as-expected right?
Unless we want to add TS-specific unit tests, then those should happen outside of Storybook because we'd be verifying that the types we defined work as we expect.
We don't want people to pass no items, it's just that sometimes they do right?
I can't remember the previous reason we added that NullElement.
Why would someone pass null to this component? There was a reason for it though. Shouldn't they just wrap the whole MenuButton in a condition instead of passing it nothing? items.length > 0?
There was a problem hiding this comment.
It always accepted falsy children just that TS complained.
It worked in JS with falsy children because the type-checking no longer applies. I don't believe it's a good dev-experience though for consumers to get TS errors and need to squash them with a ts-ignore because our typing is wrong and possibly leading to build failures on their end.
We shouldn't be adding unit tests for type checks. We should expect TS works as-expected right?
Just because TS is happy with the type, doesn't mean its typed to our expectations, hence the bug that I am addressing here.
I'm OK with removing this and I agree that a story may not be the right place for a compile-time test to prove that the type works as expected. I wanted a way to show though, that a) this works and b) that it doesn't break again.
Is there a better way going forward to include a regression test for something like this?
chrispulsinelli-okta
force-pushed
the
menu-button-accept-falsy-children
branch
from
ce37e88
to
0de83f9
Compare
|
You'll need to rebase from |
|
Closing this as it's too hard to merge now because develop vs main. New PR with all commits cherry-picked: #1999 |
|
Closing in favour of #1999 |
Description
Fixes the typing for the MenuButton component's children so that it may accept falsy-like values