-
Notifications
You must be signed in to change notification settings - Fork 8
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
refactor(pie-docs): DSW-2324 update the tag overview docs #2067
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 99e9dfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
0ab86dc
to
99e9dfc
Compare
@@ -32,7 +34,8 @@ Tags can be embedded in other components such as cards, data tables (among other | |||
dont: { | |||
type: usageTypes.text, | |||
items: [ | |||
"Don’t use when an interaction is required, use a [chip](/components/chip/) if needed." | |||
"Don’t use for interactions crucial for the flow.", | |||
"If only an icon is required to convey the message, use the tag: icon only component instead." |
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.
If this makes sense to design then no issues - but I don't really understand the wording here. Is it a figma option?
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.
Currently a figma option, when we support this in engineering, it will link to another page.
from the ticket ->
Note: the don’ts section has a link to icon only documentation. This will be implemented in DSW-2550 so for now just ignore adding the link.
} %} | ||
|
||
{% list { | ||
type: listTypes.ordered, | ||
items: [ | ||
"**Icon (Optional):** Visually supports the label.", | ||
"**Label:** Provides information about the content or purpose of the tag.", | ||
"**Container**: Background container that organises the information." | ||
"**Label:** Provides informative information to the user.", |
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.
I can feel Xander disapproving of this wording across the planet - I think "informative information" might be redundant. Could we remove "informative" please?
"**Label:** Provides informative information to the user.", | |
"**Label:** Provides information to the user.", |
"**Label:** Provides information about the content or purpose of the tag.", | ||
"**Container**: Background container that organises the information." | ||
"**Label:** Provides informative information to the user.", | ||
"**Trailing icon (Optional):** Indicates additional actions or further interactions." |
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.
Just to check we haven't missed a requirement here - does the trailing icon emit a different event to the leading?
|
||
Tag can use colour for visual categorisation. | ||
The non-interactive variation should be used by default, and should be used when the tag doesn’t require any interactive features tied to the instance. |
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.
The non-interactive variation should be used by default, and should be used when the tag doesn’t require any interactive features tied to the instance. | |
The non-interactive variation should be used by default, and should be used when the tag doesn’t require any interactivity. |
alt: "A pair of strong and subtle tags in green color." | ||
src: "../../../assets/img/components/tag/modifier-emphasis-strong.svg", | ||
width: "69px", | ||
alt: "A tag component with the strong modifier to indicate emphasis" |
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.
I don't know if this is very useful alt text. Should it describe the image a bit more?
alt: "A left-to-right example of an offer with a subtle green tag." | ||
src: "../../../assets/img/components/tag/example-ltr-restaurant-listing.svg", | ||
width: "343px", | ||
alt: "A left-to-right example of a tag used on a restaurant listing card." |
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.
I think we need proper descriptions here and the image below
Describe your changes (can list changeset entries if preferable)
Author Checklist (complete before requesting a review)
Reviewer checklists (complete before approving)
Reviewer 1
Reviewer 2