-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert Component playground(s) to use playground library #817
Conversation
This is a low-priority item — don't feel compelled to review this right now unless you've already been through everything else that's part of the current sprint. |
Visit the preview URL for this PR (updated for commit 38bea0b): https://blui-react-docs--pr817-feature-docs-convert-svc2b61q.web.app (expires Fri, 29 Dec 2023 22:33:50 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 34d39fa5aab0ea0cf95074e8e76f68829e7a8c65 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow
${ | ||
data.icon && data.icon !== 'undefined' | ||
? `icon={${(data.icon as string).replace('/>', '')}fontSize={'inherit'}${ | ||
data.htmlColor && data.htmlColor !== 'undefined' ? ` htmlColor={'${data.htmlColor as string}'}` : '' | ||
} />}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined
is a fairly common thing in our react components, given that we often want to test the component behavior when an optional prop is not given. Is it possible to incorporate this into the package? Like, for all required: false
prop data, add an extra field undefinedOption: true
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you are describing is achievable already. Each parameter that you define to show in the sidebar has the following core values configurable:
initialValue is the value to prepopulate in the sidebar input, defaultValue is the default value of the component prop if it's not defined (we use this to automatically remove values from the snippet that are the same as the default)
The 'undefined'
string being used here is because this parameter renders as a select/dropdown and there is an explicit option in there for 'undefined' (without the option, there is no way to unselect an item once you've selected one). The select input type requires option values
to be number | string
— this could probably be extended to support a value of undefined
as well if we want to be able to do a comparison like data.icon !== undefined
instead. That might complicate the typings a little bit (probably why I didn't do it initially)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I think I remember — allowing undefined
as a value in the dropdown would throw an error:
Warning: A component is changing an uncontrolled input to be controlled. This is likely caused by the value changing from undefined to a defined value, which should not happen. Decide between using a controlled or uncontrolled input element for the lifetime of the component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered providing the ability to do a value mapping from string (label in dropdown) to anything (actual value you want to parameter to have, e.g. undefined
or <Component />
), like options: [{label: 'undefined', value: undefined}]
, but that seemed to overcomplicate things for minimal gain. But we could revisit that if we want to. It might help when defining the preview component so the user doesn't have to do the mapping/lookup themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can help with both
- our testing (passing in the supposed default value vs. actually passing in undefined) and
- simplify this
generateSnippet
function that our adopters will write, like what you said there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took another look at it, and I think we'll leave it as-is for now. With the mapping capability, the user would still be stuck — e.g., In the case of the ChannelValue example, when picking an icon, the actual icon element they would pass as the value in the mapping would need to also know the value of the htmlColor input to render properly. Since the config object is static, they wouldn't be able to access that variable, so I don't think there's any real gain. That may not have made a lot of sense, but I can show you what I mean if you want to dig deeper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional to allow users to edit field 'in their workspace' rather than trying to edit fields at the very bottom of the sidebar. This is a common feature of code/text editors where users are able to 'overscroll' past the last line of text to bring it up to the middle/top of the editor.
These are unstyled accordions (i.e., default style from the theme) — what color do you want them to be, same as the background? |
the |
Everything has been migrated to the new components now...this is ready for more scrutiny |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This *
feels weird (Drawer / Drawer playground)
Dark theme, need to make the accordion and the background the same color (I don't care you set which to what, as long as the background color is the same)
Drawer header playground, the generated code snippet yields a mix of tabs and spaces.
A few generated code snippet shows the closing >
on a new line when no props are present, making my prettier
head twitch. This is drawer subheader's playground, with its default configuration:
Empty state, selecting <Router>
for the icon
prop will cause the icon to disappear (it exists as a MUI Icon and a Material Design icon)
Can we log these as bugs so we can avoid pushing changes into a 130+ file PR and make a few smaller, easier-to-review ones?
These can be logged as issues in the playground library.
These can be logged as issues in the dev-docs |
Changes proposed in this Pull Request:
AppBarChannelValueEmptyStateHeroInfoListItemListItemTagScreenshots / Screen Recording (if applicable)
To Test:
yarn start:docs
Any specific feedback you are looking for?