Skip to content
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

docs: Refreshing GlobalHeader Storybook Example #2891

Merged

Conversation

williamjstanton
Copy link
Collaborator

@williamjstanton williamjstanton commented Aug 26, 2024

Summary

Fixes: none

The Storybook example GlobalHeader had some opportunities for improved accessibility.

  1. Added accessible Tooltip to icon buttons
  2. Replaced old SearchField with Combobox
  3. Used AriaLiveRegion to demonstrate screen reader supported CountBadge
  4. Updated style packages

Release Category

Documentation, Examples

Checklist

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)
  • PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
  • Breaking Changes provides useful information to upgrade to this code or removed if not applicable

Where Should the Reviewer Start?

Areas for Feedback? (optional)

  • Code
  • Documentation
  • Testing
  • Codemods

Testing Manually

Screenshots or GIFs (if applicable)

Thank You Gif (optional)

{filteredTasks.length === 0 ? (
<StyledMenuItem as="span">No Results Found</StyledMenuItem>
) : (
filteredTasks.map(i => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although mapping works, we should probably use a render function like in the auto complete example:

{model.state.items.length > 0 && (
                <Combobox.Menu.List cs={styleOverrides.comboboxMenuList}>
                  {item => <Combobox.Menu.Item>{item}</Combobox.Menu.Item>}
                </Combobox.Menu.List>
              )}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need some help with using this model!

Copy link
Contributor

@mannycarrera4 mannycarrera4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets clean up some of the styles and code examples. Content wise it's great! Thank you as always for working through making our documentation better

const GlobalHeaderItem = createComponent('div')({
displayName: 'GlobalHeader.Item',
Component: ({gap = 's', ...props}: HeaderItemProps, ref) => (
<Flex gap={gap} alignItems="center" marginX={space.xs} ref={ref} {...props} />
<Flex gap={gap} alignItems="center" marginX={system.space.x3} ref={ref} {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does passing a token here work?

Copy link

cypress bot commented Nov 8, 2024

Workday/canvas-kit    Run #8030

Run Properties:  status check passed Passed #8030  •  git commit 2d67180555 ℹ️: Merge 06bf5c5968ec2b44d1e94dc5e5ea3eef0501ff30 into 101b613622d102220b6c00ae496e...
Project Workday/canvas-kit
Branch Review william-global-header-example-polish
Run status status check passed Passed #8030
Run duration 03m 41s
Commit git commit 2d67180555 ℹ️: Merge 06bf5c5968ec2b44d1e94dc5e5ea3eef0501ff30 into 101b613622d102220b6c00ae496e...
Committer William Stanton
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 27
Tests that did not run due to a developer annotating a test with .skip  Pending 24
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 1093
View all changes introduced in this branch ↗︎
UI Coverage  21.77%
  Untested elements 1640  
  Tested elements 454  
Accessibility  99.17%
  Failed rules  5 critical   5 serious   0 moderate   2 minor
  Failed elements 180  

@alanbsmith alanbsmith merged commit 802112a into Workday:master Nov 8, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants