-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Sarka/playroom snippets #4583
Sarka/playroom snippets #4583
Conversation
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.
Key Issues
The children
prop is being used as a function without type checking, which can lead to runtime errors if non-function React nodes are passed.
Storybook staging is available at https://kiwicom-orbit-sarka-playroom-snippets.surge.sh |
Size Change: 0 B Total Size: 459 kB
ℹ️ View Unchanged
|
|
||
const loading = `<Loading loading text="Please wait, content is loading..." type="pageLoader" />`; | ||
|
||
const navigationBar = ` |
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 should be on Layouts file, imo
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.
yeah, I was thinking about it as well. PR is just a draft
for now.
id="navigation-menu-id" | ||
// onHide={function()} | ||
// onMenuOpen={function()} | ||
// onShow={function()} | ||
openTitle="Open navigation menu" |
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.
We can delete all these lines, keep it simple
</Popover> | ||
`; | ||
|
||
const radio = `<Radio label="Label" name="Name" tabIndex={0} value="value" />`; |
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.
const radio = `<Radio label="Label" name="Name" tabIndex={0} value="value" />`; | |
const radio = `<Radio label="Label" name="Name" value="value" />`; |
spacing="100" | ||
> | ||
<ButtonLink | ||
aria-label="Favourites" |
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.
No need to have aria-label here
icon={<Icons.Accommodation />} | ||
role="list" | ||
selectable | ||
tabIndex={0} |
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.
Why this tabIndex? Same for the below one
<ListItem> | ||
24,000 locations{' '} | ||
<TextLink href="#"> | ||
around | ||
</TextLink> | ||
{' '}the globe | ||
</ListItem> |
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.
We can keep it much simpler by just having some Text, no need for a link or whatever
46880f0
to
1721892
Compare
Changed. I tried to make it as simple as possible. ✨ |
1721892
to
43065b9
Compare
This Pull Request meets the following criteria:
For new components:
d.ts
files and are exported inindex.d.ts
✨
Description by Callstackai
This PR introduces new components and their corresponding snippets for use in the playroom, including
LinkList
,List
,ListChoice
,Loading
,NotificationBadge
,Pagination
,Popover
,Radio
,Seat
, andNavigationBar
.Diagrams of code changes
Files Changed