docs: notes on brand packaging discovery #248
Conversation
Co-authored-by: Mena Hassan <[email protected]>
| .alert.alert-info .alert-icon { | ||
| color: black; | ||
| color: black; |
There was a problem hiding this comment.
Found paragon variable replacement: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L18
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #248 +/- ##
=======================================
Coverage 96.45% 96.45%
=======================================
Files 194 194
Lines 1835 1835
Branches 322 322
=======================================
Hits 1770 1770
Misses 60 60
Partials 5 5 ☔ View full report in Codecov by Sentry. |
| @@ -1,3 +1,3 @@ | |||
| .confirm-email-now-button { | |||
| text-decoration: underline !important; | |||
| text-decoration: underline !important; // Can we find paragon replacement? Can we remove !important? | |||
There was a problem hiding this comment.
Paragon replacement:
$inline-link-decoration: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L392C62-L392C62
| @@ -1,6 +1,6 @@ | |||
| @import "@edx/paragon/scss/core/core"; | |||
|
|
|||
| .course-card { | |||
| .course-card { // Can this be replaced with paragon's card component? | |||
There was a problem hiding this comment.
Should be replaced with Paragon's Card component: https://paragon-openedx.netlify.app/components/card/
| @@ -1,4 +1,4 @@ | |||
| .program-card { | |||
| .program-card { // Can entire file be replaced with paragon's card component? | |||
There was a problem hiding this comment.
Can this be replaced by Paragon's Card component? https://paragon-openedx.netlify.app/components/card/
| @@ -7,6 +8,8 @@ | |||
| font-size: 18px; | |||
| } | |||
| } | |||
|
|
|||
| // Seems to be hard coded features for a card component. Can this be replaced by paragon's card component? | |||
There was a problem hiding this comment.
Should be replaced with Paragon's Card component: https://paragon-openedx.netlify.app/components/card/
| @@ -1,3 +1,4 @@ | |||
| // Hard coded, can this be replaced by paragon? | |||
| .related-programs-modal { | |||
| .programs-title { | |||
| font-size: 1.5rem; | |||
There was a problem hiding this comment.
Not seeing an exact 1.5rem replacement but here are some alternatives that may work:
$font-size-lg: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L702C1-L702C14 (I think this would work out to 1.41 rem or so pending whatever the default is set to since it’s value is $font-size-base * 1.25 !default)$h3-font-size: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L718 (value is 1.375rem !default)
| .learner-variant-header { | ||
| a { | ||
| // needed to make the link not resize the header | ||
| // Hard coded, not sure of the best way to address this | ||
| border-bottom: 2px solid transparent; |
There was a problem hiding this comment.
Not 100% confident in these but a couple of options here:
$border-width: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L466C36-L466C36$paragraph-margin-bottom(this might be a better fit): https://github.com/openedx/paragon/blob/bd38d62df8e116cd83ad6e651d939b0e70928503/scss/core/_variables.scss#L427C17-L427C17
| .learner-variant-header { | ||
| a { | ||
| // needed to make the link not resize the header | ||
| // Hard coded, not sure of the best way to address this | ||
| border-bottom: 2px solid transparent; | ||
| } | ||
| .course-link { |
There was a problem hiding this comment.
For line 15 under this (border-bottom: 2px solid !important)
It seems to essentially be just an underline. Potential replacement is $inline-link-decoration: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L392C62-L392C62
| .nav-small-menu { | ||
| > * { | ||
| justify-content: flex-start !important; | ||
| justify-content: flex-start !important; |
There was a problem hiding this comment.
There’s not a flex-start option but this $displays variable might be an option? https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L842
|
|
||
| &::after { | ||
| content: '\00BB'; | ||
| padding-left: 10px; | ||
| padding-left: 10px; // Could potentially be replaced by paragon's styles? |
There was a problem hiding this comment.
not 100% confident but
$list-inline-padding: https://github.com/openedx/paragon/blob/bd38d62df8e116cd83ad6e651d939b0e70928503/scss/core/_variables.scss#L773$list-group-item-padding-y/$list-group-item-padding-x: https://github.com/openedx/paragon/blob/bd38d62df8e116cd83ad6e651d939b0e70928503/scss/core/_variables.scss#L860
| } | ||
| } | ||
| } | ||
|
|
||
| .logo { | ||
| // copy from legacy dashboard | ||
| height: 40px; | ||
| // Removed hard coded px height. Suggested change comes from frontend component header in order to have some consistencies. It seems to work just fine when using the same exact style that we have for logo in frontend component header |
There was a problem hiding this comment.
since Frontend Component Header is being used everywhere else, we can replace the previous pixel value with 1.75rem for the sake of being consistent
| @@ -1,5 +1,6 @@ | |||
| @import "@edx/paragon/scss/core/core"; | |||
|
|
|||
| // Hard coded, should we be using paragon equivalent? | |||
| .explore-courses-btn { | |||
| padding-top: 16px; | |||
There was a problem hiding this comment.
Not sure if this is the best fit but potential replacement here: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L341
| @@ -1,5 +1,6 @@ | |||
| @import "@edx/paragon/scss/core/core"; | |||
|
|
|||
| // This is essentially creating a card component, can be replaced by using paragon's card component | |||
There was a problem hiding this comment.
Should be replaced with Paragon's Card component: https://paragon-openedx.netlify.app/components/card/
| @@ -1,5 +1,5 @@ | |||
| @import "@edx/paragon/scss/core/core"; | |||
|
|
|||
| // Seems to be trying to create a card component. Can this entire file be replaced with paragon's card component? | |||
There was a problem hiding this comment.
To do: replace this file with Paragon's Card component and see what it looks like: https://paragon-openedx.netlify.app/components/card/
There was a problem hiding this comment.
if replacing this with the Card component doesn't work, note that there's a lot of !important values scattered around this file
| @@ -7,6 +8,8 @@ | |||
| font-size: 18px; | |||
There was a problem hiding this comment.
could be replaced by $h4-font-size: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L719C1-L720C1
| @@ -1,3 +1,4 @@ | |||
| // Hard coded max-width and width features, is this needed? And can this be replaced by paragon? | |||
| .pgn__sheet-component { | |||
| max-width: 75% !important; | |||
There was a problem hiding this comment.
I think this is a container so here are a couple of max-width options I noticed:
$container-max-widths: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L436C1-L436C22 (I thought this might be a good match initially but the options are pixel values rather than percentages so unclear if it’s the right fit)@include _assert-ascending($container-max-widths, "$container-max-widths");: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L443C1-L444C1
| @@ -18,6 +19,7 @@ | |||
| } | |||
| } | |||
|
|
|||
| // Margin-bottom is hard coded, can we replace this with paragon/bootstrap? | |||
| .masquerade-form-input { | |||
| margin-bottom: 0; | |||
There was a problem hiding this comment.
Potential replacements:
$paragraph-margin-bottom: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L427$headings-margin-bottom: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L730C2-L730C24$label-margin-bottom: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L779
| @@ -1,11 +1,11 @@ | |||
| @import "@edx/paragon/scss/core/core"; | |||
|
|
|||
| #no-courses-content-view { | |||
| border: 2px solid $light-400; | |||
| border: 2px solid $light-400; // Can this be replaced by paragon? | |||
There was a problem hiding this comment.
This only has a value of 1px but still noting this $border-width variable for reference: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L466C36-L466C36
| flex-direction: column; | ||
| padding-bottom: map-get($spacers, 5); | ||
| padding-top: map-get($spacers, 5); | ||
| height: 100%; | ||
| height: 100%; // Can this be replaced by paragon? |
There was a problem hiding this comment.
Unclear what the best replacement for height is
|
is this work still in progress or can we go ahead and close this PR? |
|
There has been no response in the last two weeks, and has been open for nearly 5 months. I'm going to close this PR for the meantime, @cintnguyen please reopen at your leisure if anything here needs to be reviewed and merged. |
Currently trying to make learner dashboard be in compliance with MFE requirements refer to docs here. Part of the requirement is using CSS utility classes in a branding friendly way. During discovery while combing through scss files we found several areas of concern. Throughout the code we've highlighted these areas with comments next to styles that are hard coded and for some of them have found Paragon variable replacements.
Need feedback on whether there are any concerns with the suggested replacements, if there are better alternatives, and suggestions for items where recommendations weren't offered.