-
Notifications
You must be signed in to change notification settings - Fork 2
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
[SAGE-396] Alert foundations update #1349
Conversation
feb1b3b
to
88ce6eb
Compare
2cc1b0b
to
45e299c
Compare
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.
🙌🏻
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! 👍🏼 - Not a blocker for this PR, but if time allows it would be great to switch out the usage of link_to
in favor of SageLink
.
Also, do the close buttons have a new hover effect with a border? Just haven't seen that one and wanted to make sure.
Overall, great work!
}, | ||
], | ||
} do %> | ||
<% content_for :sage_alert_actions do %> | ||
<%= link_to "Check usage", "#" %> | ||
<%= link_to "Check usage", "#", class: "sage-link sage-link--secondary" %> |
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.
Could we possibly switch these out for SageLink
?
<%= sage_component SageLink, {
url: "#",
label: "Check usage",
launch: false,
help_link: false,
show_label: true,
style: "secondary"
} %>
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.
Yep, you're right, that would be much better haha
<%= link_to "Action Link", "#", class: "sage-link sage-link--secondary"%> | ||
<%= link_to "Action Link", "#", class: "sage-link sage-link--secondary" %> |
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.
Could these possibly be a SageLink?
<%= link_to "Action Link", "#", class: "sage-link sage-link--secondary" %> | ||
<%= link_to "Action Link", "#", class: "sage-link sage-link--secondary" %> | ||
<%= link_to "Action Link", "#", class: "sage-link sage-link--secondary" %> |
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.
Could the also be SageLink the same as mentioned above?
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
@pixelflips Thanks for pointing out the close button hover style. I've removed it for now, but I'm consulting design on how we'd like to address this. For now, I'll file a follow-up ticket. |
@anechol Sounds great and I think a follow-up is just fine. |
5af1b0a
to
e6714d9
Compare
Description
Updates Alert component to match Figma design specs:
Figma
Implementation Updates
secondary_action
to appropriately useurl
prop.Follow-up Items
N/A
Screenshots
Testing in
sage-lib
Testing in
kajabi-products
url
for links.Related
SAGE-396