-
Notifications
You must be signed in to change notification settings - Fork 65
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
[BD-46] fix: stretch the message block to the full width #2219
[BD-46] fix: stretch the message block to the full width #2219
Conversation
Thanks for the pull request, @monteri! When this pull request is ready, tag your edX technical lead. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #2219 +/- ##
=======================================
Coverage 91.07% 91.07%
=======================================
Files 234 234
Lines 4122 4122
Branches 981 981
=======================================
Hits 3754 3754
Misses 361 361
Partials 7 7 ☔ View full report in Codecov by Sentry. |
src/Card/Card.scss
Outdated
@@ -422,6 +422,8 @@ a .pgn__card { | |||
border-radius: 0 0 .375rem .375rem; | |||
|
|||
.pgn__card-status__message-content { | |||
width: 100%; |
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.
Glad this is a simple fix. FWIW, flex-grow: 1;
here would also do the trick, and may make a bit more sense given this element doesn't actually take up 100% of the parent's width (i.e., it grows to take up whatever space is remaining after the space taken up by the icon).
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, I checked this also work fine. Don't know whether there will be difference in some cases? Just curious. I replaced to flex-grow: 1;
src/Card/Card.scss
Outdated
@@ -422,6 +422,8 @@ a .pgn__card { | |||
border-radius: 0 0 .375rem .375rem; |
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.
It looks like the padding used for Card.Status
is a bit more than the padding used for the other Card
subcomponents like Card.Footer
and Card.Section
(1.5rem vs. 1.25rem respectively). Should the padding above (i.e., padding: 1.5rem;
) be using the $card-spacer-x
and/or $card-spacer-y
variables to ensure the padding is consistent?
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.
I tested with $card-spacer-x
and $card-spacer-y
and it looks good. I agree with your comment. Worth noting that it is important to hold the logic behind spacings and sizes in the component especially in such big as Card, Datatable etc. Then it is easier to extend and modify for Paragon users.
src/Card/Card.scss
Outdated
@@ -422,6 +422,8 @@ a .pgn__card { | |||
border-radius: 0 0 .375rem .375rem; |
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.
[curious] Should we be using the $card-border-radius
SCSS variable here to be consistent with border radius throughout the Card
(sub-)components?
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 makes sense, updated
@monteri 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 20.32.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 21.0.0-alpha.25 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Add stretching the message block to the full width
Deploy Preview
Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).
Merge Checklist
example
app?wittjeff
andadamstankiewicz
as reviewers on this PR.Post-merge Checklist