-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: inherit font-size
on AlertModal
title and set line-height
on all modal titles
#3448
fix: inherit font-size
on AlertModal
title and set line-height
on all modal titles
#3448
Conversation
✅ Deploy Preview for paragon-openedx-v22 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-22.x #3448 +/- ##
=============================================
Coverage 93.40% 93.40%
=============================================
Files 252 252
Lines 4534 4534
Branches 1061 1066 +5
=============================================
Hits 4235 4235
Misses 292 292
Partials 7 7 ☔ View full report in Codecov by Sentry. |
@@ -125,6 +125,7 @@ | |||
|
|||
.pgn__modal-title { | |||
font-size: $h3-font-size; | |||
line-height: $h3-font-size * $headings-line-height; |
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.
[inform] Brand packages might override the default font-size/line-height of headings (e.g., h2
elements). As such, when using @edx/brand-edx.org
, the .pgn__modal-title
's, while the font-size
is correct per .pgn__modal-title
, the overridden line-height applied for the h2
element used for the modal title vs. relying on the changed h3
's line-height
. Explicitly calculating the line-height
for .pgn__modal-title
ensures the overridden h2
element line-height
does not apply to modal titles.
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.
Note: when this change gets ported to v23, the SCSS variables will need to change to the corresponding CSS variables, and likely using calc(...)
.
@@ -310,7 +311,6 @@ | |||
} | |||
|
|||
.pgn__modal-title { | |||
font-size: $h4-font-size; |
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.
[inform] Per Figma, it appears all modal dialog types (including AlertModal
should have title font size of $h3-font-size
.
font-size
on AlertModal
title and set line-height
on all modal titles.
font-size
on AlertModal
title and set line-height
on all modal titles.font-size
on AlertModal
title and set line-height
on all modal titles
🎉 This PR is included in version 22.15.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
#3129
The
AlertModal
title was previously styled with$h4-font-size
, despite the Figma showing it styled as anh3
. The default modal title font size is$h3-font-size
, so removing the$h4-font-size
falls back to the standard$h3-font-size
instead, as expected.[context] This has often led to consuming MFEs to try to override the default
AlertModal
titles on their end vs. fixing the problem upstream (e.g., nesting anh3
within the renderedh2
leading to invalid HTML, not using thetitle
prop, etc.).Figma
Original
AlertModal
Left:
@openedx/brand-openedx
Right:
@edx/brand-edx.org
Note the
h4
-styled title and line-height issue with@edx/brand-edx.org
.Updated
AlertModal
Left:
@openedx/brand-openedx
Right:
@edx/brand-edx.org
Note the
h3
-styled title and resolved line-height issue with@edx/brand-edx.org
.Deploy Preview
https://deploy-preview-3448--paragon-openedx-v22.netlify.app/components/modal/alert-modal/
Merge Checklist
example
app?Post-merge Checklist