Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Add support for List item tags and link types #4287

Merged
merged 13 commits into from
May 7, 2020

Conversation

mattsherman
Copy link
Contributor

@mattsherman mattsherman commented May 4, 2020

Infrastructure work in preparation for #4082 (Quick Links)

  • General
    • Adds support for showing console.log messages in Storybook, making it easy to verify interactions (added @storybook/addon-console as a dev dependency)
  • Link component
    • Adds Storybook stories for all Link types (wp-admin, wc-admin, external)
    • Adds unit tests for Link component
  • List component
    • Adds setting of List item link type
    • Adds onListItemClick prop
    • Adds listItemTag item prop
    • Adds item click and tag example to Storybook
    • Adds unit tests for List component

See draft PR #4292 for a look at how these changes to List will be used by the QuickLinks component in the new home screen. In particular, the item tagging and click support will be used to hook in Tracks events (not implemented yet in #4292).

Screenshots

ScreenCapture at Tue May 5 15:14:29 EDT 2020

Detailed test instructions:

Storybook

npm run storybook

Unit testing

npm test -- -i packages/components/src/link/test/index.js packages/components/src/list/test/index.js

Optional: Confidence check

  • Make sure that List changes haven’t affected other use of List in wc-admin, such as task list

Changelog Note:

  • Infrastructure work - no changelog note needed

@mattsherman mattsherman self-assigned this May 4, 2020
@mattsherman mattsherman marked this pull request as draft May 4, 2020 21:36
@mattsherman mattsherman marked this pull request as ready for review May 5, 2020 19:23
@mattsherman mattsherman requested a review from a team May 5, 2020 19:24
Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Nice thorough work here @mattsherman !

Just a couple thoughts on links, although I may be not understanding correctly here. Its also nice to see some test coverage too. I found this article really helpful and noted a few things that I learned: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library

I'm pretty impressed with how well getByRole works and how it mimics how a user or screen reader would scan a page. Just a few suggestions, but pre-approving.

packages/components/src/link/test/index.js Show resolved Hide resolved
packages/components/src/link/test/index.js Show resolved Hide resolved
packages/components/src/list/test/index.js Outdated Show resolved Hide resolved
packages/components/src/list/test/index.js Outdated Show resolved Hide resolved
@mattsherman mattsherman merged commit 023824d into master May 7, 2020
@mattsherman mattsherman deleted the update/list-with-link-type branch May 7, 2020 15:52
@mattsherman
Copy link
Contributor Author

Thanks for the review @psealock! I addressed all of your feedback prior to merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants