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

feat(combobox): Allow grouping of listbox items #535

Merged
merged 26 commits into from
Jul 10, 2020

Conversation

vibdev
Copy link
Contributor

@vibdev vibdev commented Mar 26, 2020

Summary

Adds groups for combobox. This is WIP so accessibility team can audit.

Checklist

  • branch has been rebased on the latest master commit
  • tests are changed or added
  • yarn test passes
  • all (dev)dependencies that the module needs is added to its package.json
  • code has been documented and, if applicable, usage described in README.md
  • module has been added to canvas-kit-react and/or canvas-kit-css universal modules, if
    applicable
  • design approved final implementation
  • a11y approved final implementation
  • code adheres to the API & Pattern guidelines

Additional References

@cypress
Copy link

cypress bot commented Mar 26, 2020



Test summary

427 0 1 0


Run details

Project canvas-kit
Status Passed
Commit fd932b4 ℹ️
Started Jul 9, 2020 11:42 PM
Ended Jul 9, 2020 11:47 PM
Duration 04:30 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@anicholls anicholls added the blocked This issue is waiting for one or more issues to be closed/resolved. label Apr 9, 2020
@samsmith-workday
Copy link

Reviewed Matt's work with NVDA/JAWS. Sounds good to me.

@vibdev vibdev marked this pull request as ready for review May 5, 2020 23:00
@vibdev vibdev changed the title WIP: feat(combobox): Allow grouping of listbox items feat(combobox): Allow grouping of listbox items May 5, 2020
);
};

export default Status;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Broke this out, might be nice for it to be its own component at some point.

@anicholls anicholls added ready for review Code is ready for review and removed blocked This issue is waiting for one or more issues to be closed/resolved. labels May 6, 2020
.eslintrc.js Outdated Show resolved Hide resolved
@NicholasBoll NicholasBoll self-assigned this Jun 9, 2020
Comment on lines 25 to 30
// TODO: useContext should not be called conditionally,
// but to keep this function working we need to ignore till a refactor can happen.
// Ideally there is a getTheme and a useTheme separately where getTheme is used outside of functional components
// and useTheme is used inside functional components. Then this try won’t be necessary.
// eslint-disable-next-line react-hooks/rules-of-hooks
const context = React.useContext(ThemeContext);
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this

# Conflicts:
#	modules/_labs/combobox/react/lib/Combobox.tsx
#	modules/_labs/combobox/react/stories/stories.tsx
#	modules/_labs/core/react/lib/theming/useTheme.ts
@jpante jpante added this to In Progress in Current Sprint (7/20 - 8/9) via automation Jun 23, 2020
@jpante jpante moved this from In Progress to Needs Review in Current Sprint (7/20 - 8/9) Jun 25, 2020
* Fixed some various bugs while writing tests
* Removed Jest tests that are now covered by Cypress
@NicholasBoll
Copy link
Member

Paired with @vibdev to add Cypress tests, fix various bugs found while writing tests and removed Jest tests that are now covered with Cypress

@@ -1,5 +1,6 @@
{
"projectId": "odida5",
"baseUrl": "http://localhost:9001",
"supportFile": "cypress/support/index.ts"
"supportFile": "cypress/support/index.ts",
"watchForFileChanges": false
Copy link
Member

Choose a reason for hiding this comment

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

I added this. I think it's annoying to write a bit of test code, then go back to Cypress to find out what I'm querying the DOM for and the Cypress test starts over again increasing time to write tests rather than decreasing it.

@NicholasBoll NicholasBoll added approved Code has been reviewed and approved (ship it) and removed ready for review Code is ready for review labels Jul 9, 2020
@NicholasBoll NicholasBoll mentioned this pull request Jul 9, 2020
@NicholasBoll NicholasBoll merged commit 0f6b303 into Workday:master Jul 10, 2020
Current Sprint (7/20 - 8/9) automation moved this from Needs Review to Done Jul 10, 2020
@NicholasBoll NicholasBoll mentioned this pull request Jun 1, 2021
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Code has been reviewed and approved (ship it)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants