-
Notifications
You must be signed in to change notification settings - Fork 81
Fix notification bar link spacing #1040
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
Conversation
|
After explicitly adding the missing CTA class, the JS change is now not mandatory for this to still be effective and without regressions (and is only kept as a nice-to-have for consistency) — if it would make it easier to review this fix without any |
maureenlholland
left a comment
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.
r+ ✨
good find, good fix, thanks!
| text-decoration: none; | ||
| } | ||
|
|
||
| &.mzp-c-notification-bar-cta { |
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.
👍 definitely a good idea to scope this specifically to the CTA links
| class: mzp-t-warning | ||
| close_button: True | ||
| content: Warning! It's dangerous to go alone. | ||
| content: Warning! It’s <a href="#">dangerous</a> to go alone. |
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 like this needs a rebase |
|
@alexgibson Conflicts resolved via merge UI and CI passes (with no other conflict visible from my side currently). If you'd like a rebase instead LMK. OR: Whenever you'd like to see the history edited out and cleaned up, just let me know, happy to fix up. I just usually keep the original mess before squashing to help reviewers follow the steps taken or changes backed out for some reason; so unless necessary, I don't overwrite the history just to appear "tidy", esp. in this case it's a bit more tiresome than what it would normally warrant;) — so if you don't plan on squashing and would like a better history instead I'm also happy to amend. |
Description
Removes margin that breaks natural flow of text in case the link is inside a sentence.
CHANGELOG.md.Issue
Fixes #1000 (+also resolves #1041)
Testing
Adds support for explicit
.mzp-c-notification-bar-ctaso these two render the same:https://mzp-dev.netlify.app/components/detail/notification-bar--error
https://mzp-dev.netlify.app/components/detail/notification-bar--scripted
Adding inline link example here to demonstrate other than explicit CTAs now flow naturally:
https://mzp-dev.netlify.app/components/detail/notification-bar--warning