-
Notifications
You must be signed in to change notification settings - Fork 77
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(link): add component tokens #10689
Conversation
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
# Conflicts: # packages/calcite-components/src/components.d.ts # packages/calcite-components/src/components/link/link.e2e.ts
@alisonailea @macandcheese can I get a review of this one? |
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, just some small q / notes
|
||
describe("theme", () => { | ||
describe("default", () => { | ||
themed(html` <calcite-link href="#" icon-start="banana" icon-end="information">link</calcite-link> `, { |
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.
Not sure how granular we want to get with these - you could also test against an internal span
element rendered without a href
- don't know if that's totally duplicative with this or not.
* These properties can be overridden using the component's tag as selector. | ||
* | ||
* @prop --calcite-link-text-color: Specifies the component's text color. | ||
*/ |
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.
Not actionable: I think this single property works for the time being - in the future I could see the need for --calcite-link-icon-start-color
and --calcite-link-icon-end-color
.
Users can currently just use --calcite-icon-color
- but if they wanted granular control over each icon for whatever reason, we can add those down the road as needed.
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.
Yeah, down the road.
relative | ||
inline | ||
border-none | ||
bg-transparent | ||
p-0; | ||
color: var(--calcite-link-text-color, var(--calcite-color-text-link)); | ||
line-height: inherit; | ||
white-space: initial; | ||
background-image: linear-gradient(currentColor, currentColor), |
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.
For this underline, maybe we could pass it through with --calcite-link-underline
? Kind of a strange prop, I know design was maybe going to re-evaluate this, but for now we'd need to let users override it somehow. If they've overridden the link color they'll need to change this right?
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.
Lets just think about it. we can add it later.
Related Issue: #7180
Summary
Add
link
component tokens.--calcite-link-text-color
: Specifies the component's text color.