-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add "Link::Inline" component (05) #231
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
6adf95c
to
95f9a5c
Compare
4469dfd
to
324d7e2
Compare
92b39ce
to
8c8e2ce
Compare
@cveigt in theory yes, but in practice I have two counterpoints:
what do you think @heatherlarsen ? |
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.
Technically this looks good – happy for it to be merged in the feature branch when the design comments are resolved.
# Conflicts: # packages/components/addon/components/hds/interactive/index.hbs # packages/components/app/styles/@hashicorp/design-system-components.scss # packages/components/tests/acceptance/percy-test.js # packages/components/tests/dummy/app/templates/index.hbs
# Conflicts: # packages/components/tests/dummy/app/templates/index.hbs
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.
LGTM 👍 -I like how the focus ring ended up looking. It looks better in the browser than in Figma with the same offset.
📌 Summary
This PR intends to add the missing counter-part of the
Link::Standalone
(a link that's used in isolation, not inline with adjacent text) component: theLink::Inline
(a link that's used within a body of text).Why not use just a
<a>
element?Because in this way:
Hds::Interactive
component to handle the different kinds of Ember links we need to support (<a>
,<LinkTo>
,<LinkToExternal>
)primary
("action" color) andsecondary
(dark gray) should be used)hover/active/focus
)🛠️ Detailed description
In this PR I have:
Link::Inline
componentInteractive
component🚨 Known issues
outline
: this will make it consistent across all the browsers, apart from the border radius in Safari (something that probably can't be fixed anyway)🔗 Links
TODOs
icon
in the component, or we want to remove it - see [WIP] Addicon
support to "Link::Inline" component #255👀 How to review
👉 Review by files changed
Reviewer's checklist:
💬 Please consider using conventional comments when reviewing this PR.