Skip to content

Conversation

@janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Mar 18, 2025

Description

Removes margin that breaks natural flow of text in case the link is inside a sentence.

  • I have documented this change in the design system.
  • I have recorded this change in CHANGELOG.md.

Issue

Fixes #1000 (+also resolves #1041)

Testing

Adds support for explicit .mzp-c-notification-bar-cta so 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

@stephaniehobson stephaniehobson added the Needs:Review 👋 Ready for Developer Review label Mar 19, 2025
@janbrasna
Copy link
Contributor Author

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 MzpNotification tweaks I'm happy to spin them off to a separate PR for later.

@maureenlholland maureenlholland self-assigned this Apr 15, 2025
@maureenlholland maureenlholland removed the Needs:Review 👋 Ready for Developer Review label Apr 15, 2025
Copy link
Collaborator

@maureenlholland maureenlholland left a 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 {
Copy link
Collaborator

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@alexgibson
Copy link
Contributor

Looks like this needs a rebase

@janbrasna
Copy link
Contributor Author

janbrasna commented Apr 15, 2025

@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.

@alexgibson alexgibson merged commit d34e087 into mozilla:main Apr 15, 2025
1 check passed
@janbrasna janbrasna deleted the fix/notif-links branch April 15, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MzpNotification adds cta anchor after the paragraph Notification bar padding of links only works for complete sentences

4 participants