-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI: VAULT-17026 Dashboard Learn More Card #21387
UI: VAULT-17026 Dashboard Learn More Card #21387
Conversation
|
||
/** | ||
* @module DashboardLearnMoreCard | ||
* DashboardLearnMoreCard component are used to display external links |
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.
Thank you for the documentation!
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.
Nothing blocking, just a couple of questions.
export default class DashboardLearnMoreCard extends Component { | ||
get learnMoreLinks() { | ||
return [ | ||
{ |
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.
Ah, interesting. Okay, so we have a DocLinks
component which makes it so you don't have to use the full URL. Example below. However, I see you're using a HDS link component. Any chance HDS accounts for doc links? It can't just be a thing we need. If they don't, maybe make a comment here about this or in the documentation above, and put a request in to HDS to account for it?
<DocLink @path="/vault/tutorials/auth-methods/multi-factor-authentication">
Learn more
</DocLink>
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 to know! Looking at HDS documentation it doesn't look like it takes into account DocLinks - it dynamically lets you pass in a @route
or @href
.
@@ -87,6 +87,10 @@ | |||
margin-bottom: -$spacing-m; | |||
} | |||
|
|||
.has-bottom-margin-xxs { | |||
margin-bottom: $spacing-xxs !important; |
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.
Did you test without the important? I know we have importants everywhere in our helpers, and even bulma had importants, but the feedback I got on the bulma work was do add them if we don't need them.
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! It doesn't work without !important
since the title class has a margin-bottom and takes precedence over this helper class. Do you recommend creating a stylesheet for the landing page instead of adding a helper with !important
?
@@ -40,4 +40,17 @@ module('Acceptance | landing page dashboard', function (hooks) { | |||
assert.dom('[data-test-secrets-engines-row="nomad"] [data-test-view]').doesNotExist(); | |||
}); | |||
}); | |||
|
|||
module('learn more card', function () { |
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.
nice test ✨
Description