Skip to content
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.

Sign up for GitHub

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

docs: notes on brand packaging discovery #248

Closed
wants to merge 1 commit into from
Closed

docs: notes on brand packaging discovery #248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/App.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ $input-focus-box-shadow: $input-box-shadow; // hack to get upgrade to paragon 4.
white-space: nowrap;
}

// Hard coded color here, there's a paragon equivalent
.alert.alert-info .alert-icon {
color: black;
color: black;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found paragon variable replacement: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L18

}

#root {
Expand All @@ -34,6 +35,7 @@ $input-focus-box-shadow: $input-box-shadow; // hack to get upgrade to paragon 4.
flex-grow: 1;
}

// Brand logo header has no way of being overwritten by the imported brand package therefore not customizable. Is this intentional?
header {
flex: 0 0 auto;

Expand All @@ -57,11 +59,7 @@ $input-focus-box-shadow: $input-box-shadow; // hack to get upgrade to paragon 4.
}

#paragon-portal-root {
.pgn__modal-layer {
.pgn__modal-close-container {
right: 1rem !important;
}
}
// Removed because there was no visible difference within the element with or without it
.confirm-modal .pgn__modal-body {
overflow: hidden;
}
Expand Down
2 changes: 1 addition & 1 deletion src/containers/CourseCard/CourseCard.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@import "@edx/paragon/scss/core/core";

.course-card {
.course-card { // Can this be replaced with paragon's card component?
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be replaced with Paragon's Card component: https://paragon-openedx.netlify.app/components/card/

.card {
.pgn__card-wrapper-image-cap.vertical {
display: flex;
Expand Down
3 changes: 3 additions & 0 deletions src/containers/CourseFilterControls/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a container so here are a couple of max-width options I noticed:

width: 75% !important;
Expand All @@ -7,6 +8,8 @@
font-size: 18px;
Copy link
Contributor

Choose a reason for hiding this comment

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

could be replaced by $h4-font-size: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L719C1-L720C1

}
}

// Seems to be hard coded features for a card component. Can this be replaced by paragon's card component?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be replaced with Paragon's Card component: https://paragon-openedx.netlify.app/components/card/

#course-filter-controls-card {
width: 512px;
height: 288px;
Expand Down
4 changes: 2 additions & 2 deletions src/containers/CourseList/NoCoursesView/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

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?
Copy link
Contributor

@httpsmenahassan httpsmenahassan Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unclear what the best replacement for height is


& > * {
margin-top: map-get($spacers, 3);
Expand Down
2 changes: 1 addition & 1 deletion src/containers/LearnerDashboardHeader/ConfirmEmailBanner/ConfirmEmailBanner.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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?
Copy link
Contributor

@httpsmenahassan httpsmenahassan Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Paragon replacement:

$inline-link-decoration: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L392C62-L392C62

}
27 changes: 20 additions & 7 deletions src/containers/LearnerDashboardHeader/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
// Seems to not be used anywhere, can this be removed?
.dropdown-menu-collapse {
width: 100vw;
position: absolute;
left: 0;
}

// Hard coded border bottom to indicate which tab we are on. The transparent border bottom is to align all the navigation links in the headers. Seems like a similar affect could be accomplishged using margin.
.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;
Copy link
Contributor

@httpsmenahassan httpsmenahassan Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not 100% confident in these but a couple of options here:

}
.course-link {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Expand All @@ -18,21 +20,32 @@
}
}

// Hard coded many items in the nav collapsed menu, would be hard to style
.nav-small-menu {
> * {
justify-content: flex-start !important;
justify-content: flex-start !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


border-radius: 0 !important;
border-top: 1px solid #ddd !important;
border-radius: 0 !important; // Could potentially be replaced by paragon's styles?
border-top: 1px solid #ddd !important; // Could potentially be replaced by paragon's styles?

&::after {
content: '\00BB';
padding-left: 10px;
padding-left: 10px; // Could potentially be replaced by paragon's styles?
Copy link
Contributor

Choose a reason for hiding this comment

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

not 100% confident but

}
}
}

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

@httpsmenahassan httpsmenahassan Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

https://github.com/openedx/frontend-component-header/blob/9a5b1fa5e78e72f44f1591e7b759526b2633e7ec/src/index.scss#L83

display: block;
box-sizing: content-box;
position: relative;
top: -.05em;
height: 1.75rem;
padding: 1rem 0;
margin-right: 1rem;
img {
display: block;
height: 100%;
}
}
2 changes: 2 additions & 0 deletions src/containers/MasqueradeBar/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
padding: map-get($spacers, 3);
margin-bottom: map-get($spacers, 2);

// Margin-bottom is hard coded, can we replace this with paragon/bootstrap?
.masquerade-form-label {
padding: map-get($spacers, 2) map-get($spacers, 3);
display: flex;
Expand All @@ -18,6 +19,7 @@
}
}

// Margin-bottom is hard coded, can we replace this with paragon/bootstrap?
.masquerade-form-input {
margin-bottom: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential replacements:

flex-grow: 1;
Expand Down
2 changes: 1 addition & 1 deletion src/containers/RelatedProgramsModal/components/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.program-card {
.program-card { // Can entire file be replaced with paragon's card component?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be replaced by Paragon's Card component? https://paragon-openedx.netlify.app/components/card/

color: white !important;
.program-card-banner {
.pgn__card-image-cap {
Expand Down
1 change: 1 addition & 0 deletions src/containers/RelatedProgramsModal/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// Hard coded, can this be replaced by paragon?
.related-programs-modal {
.programs-title {
font-size: 1.5rem;
Copy link
Contributor

@httpsmenahassan httpsmenahassan Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not seeing an exact 1.5rem replacement but here are some alternatives that may work:

Expand Down
1 change: 1 addition & 0 deletions src/widgets/ProductRecommendations/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be replaced with Paragon's Card component: https://paragon-openedx.netlify.app/components/card/

$horizontal-card-gap: 20px;
$vertical-card-gap: 24px;

Expand Down
2 changes: 1 addition & 1 deletion src/widgets/RecommendationsPaintedDoorBtn/components/PaintedDoorModal/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
@media (max-width: 464px) {
.pgn__action-row{
flex-direction: column;
gap: 5px;
gap: 5px; // Can this be replaced?
button {
width: 100%;
}
Expand Down
2 changes: 1 addition & 1 deletion src/widgets/RecommendationsPanel/components/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

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

To do: replace this file with Paragon's Card component and see what it looks like: https://paragon-openedx.netlify.app/components/card/

Copy link
Contributor

Choose a reason for hiding this comment

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

if replacing this with the Card component doesn't work, note that there's a lot of !important values scattered around this file

.card-link{
display: block !important;
margin: 0.5rem 0 0.5rem 0 !important;
Expand Down
2 changes: 2 additions & 0 deletions src/widgets/RecommendationsPanel/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
@import "@edx/paragon/scss/core/core";

// Hard coded, should we be using paragon equivalent?
.explore-courses-btn {
padding-top: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is the best fit but potential replacement here: https://github.com/openedx/paragon/blob/0e6631bd1f26edc2871c522ad88ffba33bc32617/scss/core/_variables.scss#L341

}
Expand All @@ -8,6 +9,7 @@
background: $light-200;
}

// Height value is hard coded. Would we we want poeple to be able to change the height?
.recommendations-loading {
display: flex;
justify-content: center;
Expand Down