feat: skip links #3317
Merged
feat: skip links #3317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
🦋 Changeset detectedLatest commit: 325bd53 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
Status: Do Not Merge
github-actions
bot
added
Type: Documentation
✅ Deploy Preview for paste-theme-designer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for paste-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 325bd53:
|
Passing run #6246 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
|
Size Change: +575 B (0%) Total Size: 1.02 MB
ℹ️ View Unchanged
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 325bd53. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
SiTaggart
commented
Jul 11, 2023
| AccountSwitcherBadge.displayName = 'AccountSwitcherBadge'; | ||
| AccountSwitcherBadge.propTypes = MenuBadgePropTypes; | ||
| AccountSwitcherBadge.propTypes = accountSwitcherPropTypes; |
There was a problem hiding this comment.
Throwing errors in test and storybook
SiTaggart
commented
Jul 11, 2023
| @@ -18,7 +18,7 @@ const MenuItem = React.forwardRef<HTMLDivElement, MenuItemProps>( | |||
|
|
|||
| export const MenuItemPropTypes = { | |||
| href: PropTypes.string, | |||
| variant: PropTypes.oneOf([Object.values(MenuItemVariants)]), | |||
| variant: PropTypes.oneOf(Object.values(MenuItemVariants)), | |||
There was a problem hiding this comment.
Throwing errors in test and storybook
SiTaggart
commented
Jul 11, 2023
| @@ -18,7 +18,7 @@ const MenuItemCheckbox = React.forwardRef<HTMLDivElement, MenuItemCheckboxProps> | |||
|
|
|||
| export const MenuItemCheckboxPropTypes = { | |||
| href: PropTypes.string, | |||
| variant: PropTypes.oneOf([Object.values(MenuItemVariants)]), | |||
| variant: PropTypes.oneOf(Object.values(MenuItemVariants)), | |||
There was a problem hiding this comment.
Throwing errors in test and storybook
SiTaggart
commented
Jul 11, 2023
| ProductSwitcherButton.displayName = 'ProductSwitcherButton'; | ||
| ProductSwitcherButton.propTypes = MenuButtonPropTypes; | ||
| ProductSwitcherButton.propTypes = productSwitcherButtonPropTypes; |
There was a problem hiding this comment.
Throwing errors in test and storybook
SiTaggart
commented
Jul 11, 2023
SiTaggart
commented
Jul 11, 2023
| render(<CustomizedUserDialog />); | ||
| await waitFor(() => { | ||
| render(<CustomizedUserDialog />); | ||
| }); |
TheSisb
reviewed
Jul 11, 2023
| @@ -16,6 +16,8 @@ const AccountSwitcherBadge = React.forwardRef<HTMLButtonElement, AccountSwitcher | |||
| } | |||
| ); | |||
|
|
|||
| // omit variant from MenuBadgePropTypes because it conflicts with AccountSwitcherBadge | |||
| const {variant, ...accountSwitcherPropTypes} = MenuBadgePropTypes; | |||
There was a problem hiding this comment.
question: No error for variant being unused?
There was a problem hiding this comment.
We set it in the component above. AccountSwitcher is a subset of Menu, So I want to reuse the proptypes from Menu as much as I can, but because we set variant inside account switcher, the user can't so I don't want to error for them
packages/paste-core/components/sidebar/src/navigation/SidebarNavigation.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
| }); | ||
| export const Topbar = React.forwardRef<HTMLDivElement, TopbarProps>( | ||
| ({element = 'TOPBAR', topbarSkipLinkID, children, ...props}, ref) => { |
There was a problem hiding this comment.
question: I was thinking the prop would just be id: string (required), did you name it this way so people knew there was a relationship here?
There was a problem hiding this comment.
That was the theory, to be more explicit on why we need it. My initial fear was that if it's just id, when we require it with typescript, there's nothing really telling the person that its anything related to the Sidebar and skip links. It's just "giev me an id"
We could TSDoc it on the property to give a VS Code hint, but requires the person to read the popup.
SiTaggart
force-pushed
the
feat/sidebar-skip-links
branch
from
c32d1b3
to
55fdff1
Compare
TheSisb
previously approved these changes
Jul 12, 2023
SiTaggart
added
🕵🏻♀️ Run website visual regression
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description and useful links to discussions / issues / tickets
Changes in this PR:
Skip links to Sidebar
Add the ability to have hidden skip links that are the first things in the DOM for navigating a complex application.
Semantics update for Sidebar
Change sidebar to
asideand Sidebar Nav to be thenavelementDocument the Skip links functionality
In Sidebar and Topbar docs document the skips
Topbar actions docs
Add the docs for the new Topbar Actions components.
User dialog tests
Fixed a very noisy test for user dialog
Checklist
requiredchecks have passed🕵🏻♀️ Run website visual regressionlabel