-
Notifications
You must be signed in to change notification settings - Fork 221
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
fix: Add Side Panel with Navigation Example #2106
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
1 flaky tests on run #5759 ↗︎
Details:
cypress/integration/Autocomplete.spec.ts • 1 flaky testThis comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
modules/react/_examples/stories/SidePanelWithNavigation.stories.mdx
Outdated
Show resolved
Hide resolved
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
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 is looking great!! I left one comment of feedback on the aria-label string for you. The list markup looks like it is working really well!
modules/react/_examples/stories/examples/SidePanelWithNavigation.tsx
Outdated
Show resolved
Hide resolved
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.
Apologies in advance... I should have spotted this in my last review. I just noticed the second expandable did not have the list markup like the first expandable did.
</Expandable.Title> | ||
<Expandable.Icon iconPosition="end" flex="1" /> | ||
</StyledExpandable> | ||
<Expandable.Content paddingY="zero" paddingX="zero"> |
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.
Apologies for missing this earlier in the morning! This line is missing as="ul" prop.
<Expandable.Content paddingY="zero" paddingX="zero"> | ||
{cakes.map(item => { | ||
return ( | ||
<StyledLink |
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.
And, these StyledLink components are not set inside of <li>
elements.
</Expandable.Title> | ||
<Expandable.Icon iconPosition="end" flex="1" /> | ||
</StyledExpandable> | ||
<Expandable.Content paddingY="zero" paddingX="zero"> |
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.
<Expandable.Content paddingY="zero" paddingX="zero"> | |
<Expandable.Content paddingY="zero" paddingX="zero" as="ul"> |
Summary
Fixes: #1910
Release Category
Examples
Checklist
ready for review
has been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)