fix: restore authz bg styles but keep content cap to header xl - #235
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @dcoa! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #235 +/- ##
=======================================
Coverage 98.53% 98.53%
=======================================
Files 73 73
Lines 1297 1299 +2
Branches 394 396 +2
=======================================
+ Hits 1278 1280 +2
Misses 19 19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
72d4376 to
d99430c
Compare
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
Approved, but with a few suggestions. Address them at your convenience.
| <Card> | ||
| // The matrix is wider than a narrow viewport once it has a few role columns, so the | ||
| // card scrolls rather than letting the table overflow with no way to reach it. | ||
| <Card className="overflow-auto"> |
There was a problem hiding this comment.
overflow-auto makes the Card the scroll container for the sticky-top headers below, so they no longer pin to the viewport at any width. Scope the scroll to small breakpoints with a media query, or drop sticky-top.
| <div className="col col-md-7"> | ||
| <p className="text-primary font-weight-bold h4">{intl.formatMessage(messages['authz.tabs.permissionsRoles.courses.alert.title'])}</p> | ||
| <span> | ||
| <span className="font-weight-bold">{intl.formatMessage(messages['authz.tabs.permissionsRoles.courses.alert.note'])}</span> | ||
| {intl.formatMessage(messages['authz.tabs.permissionsRoles.courses.alert.description'])} | ||
| </span> | ||
| </div> | ||
| <div className="col col-5"> | ||
| <div className="col col-md-5 mt-3 mt-md-0"> |
There was a problem hiding this comment.
.col is flex: 1 0 0, so these still sit 50/50 below md (and flex-wrap is already the .row default). col-12 col-md-7 / col-12 col-md-5 will stack them.
| return ( | ||
| <Container className="p-5"> | ||
| <> | ||
| <Container className="pb-5"> |
There was a problem hiding this comment.
With the outer Container gone, this one is centred with Paragon's breakpoint max-widths, so the toggle drifts up to ~100px right of the table at tablet widths. A plain <div className="pb-5"> fixes it.
| {typeof pageSubtitle === 'string' | ||
| ? <> { pageSubtitle !== '' && <hr className="mx-lg-3" /> }<h3 className="mb-0 py-2 font-weight-light text-gray-700">{pageSubtitle}</h3></> | ||
| : <>{ pageSubtitle !== '' && <hr className="mx-lg-3" /> } <div className="mb-0">{pageSubtitle}</div></>} | ||
| ? <> { pageSubtitle !== '' && <hr className="authz-action-divider mx-lg-3" /> }<h3 className="mb-0 py-2 font-weight-light text-gray-700">{pageSubtitle}</h3></> |
There was a problem hiding this comment.
Nit: with no subtitle this still renders an empty <h3> (axe empty-heading). Consider rendering nothing when pageSubtitle is falsy.
| // element stays full width, so a background it carries still bleeds to the edges. Must | ||
| // span the viewport: `100%` resolves against its own containing block, so nested in a | ||
| // constrained box the overhang silently goes to zero. Pair with `py-*`, not `p-*`, and | ||
| // not on a Paragon `Container` -- its gutter ties on specificity and wins on order. |
There was a problem hiding this comment.
Nit: frontend-base wraps Paragon in @layer paragon and app CSS in a higher layer, so .page-band would beat the Container gutter regardless of order. The py-* advice holds only because .p-* is !important.
|
thanks @arbrandes i addressed your feedback. If there is not additional comment I will merge this tomorrow. |
|
🎉 This PR is included in version 1.0.0-alpha.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
Summary
01fdfb5wrapped the app body inContainer size="xl" fluid px-0so it would line up with the site header. That capped the whole body at the header's max width, which also clipped the authz module's colour bands:bg-light-100behind the title andbg-light-200behind the content stopped bleeding to the viewport edges, leaving the page background showing down both sides above 1464px.This restores the full-bleed bands while keeping the alignment, and consolidates the page chrome that had drifted apart across the module's four pages.
Also fixed
contextprop removed fromAuthZLayout: all three call sites passed{ id: '', title: '', org: '' }for use in theStudioHeader, but that was not removed after the migration to frontend-base.pageSubtitleis now optional. Onlyaudit-useruses it; the others passed""purely to satisfy the type.