-
Notifications
You must be signed in to change notification settings - Fork 6
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
DSD-1726 & DSD-1765: TagSet style updates #1575
DSD-1726 & DSD-1765: TagSet style updates #1575
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/theme/components/tagSet.ts
Outdated
textDecorationStyle: "dotted", | ||
textDecorationThickness: "1px", | ||
textUnderlineOffset: "2px", |
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.
These are the same base styles for the Link
component. Any way we can extract those styles out and reuse an object for both?
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.
Yes, and I think I've used this in other spots as well. I've been thinking about making it a reusable snippet, but I just haven't gopne through with it...yet. Let me see what I can do.
type: "Update", | ||
affects: ["Documentation", "Styles"], | ||
notes: [ | ||
"Updated the styles for the UI colors, text treatment and `Clear all` button.", |
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 personal but how do you feel about using the oxford comma here?
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 am recent convert to using the Oxford comma. I am trying to use it more and more, but I missed this one. :) Admittedly, however, it does still bother me in certain situations.
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.
Looks good. When I first looked at the explore TagSet
, it appeared as if there would be color contrast issues but that's just me.
Added comments but not blockers. |
@EdwinGuzman I know you arleady approved, but please take a look at the last round of changes that I pushed. Thanks. |
}); | ||
|
||
it("returns 'clearFilters' when the 'Clear Filters' button is clicked", () => { | ||
it("returns 'clearFilters' when the 'Clear filters' button is clicked", () => { |
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.
Nit: is 'clearFilters' the right word here? Is this basically testing whether the correct tagSet is passed to the onClick function when 'Clear filters' is clicked? If so, maybe this would be more accurate?
"returns the appropriate tag setwhen the 'Clear filters' button is clicked"
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.
Good catch. I made an adjustment.
Fixes JIRA ticket DSD-1726 & DSD-1765
This PR does the following:
Clear all
button in theTagSet
component.TagSet
component to"Content Display"
.How has this been tested?
Accessibility concerns or updates
Checklist:
Front End Review: