-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: ENT-7367 Added skills quiz v2 #911
Conversation
45b4217
to
5b65a77
Compare
79a3f80
to
5a79d78
Compare
5a79d78
to
49dcdc1
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #911 +/- ##
==========================================
+ Coverage 84.87% 84.91% +0.03%
==========================================
Files 320 343 +23
Lines 6399 7277 +878
Branches 1552 1779 +227
==========================================
+ Hits 5431 6179 +748
- Misses 941 1065 +124
- Partials 27 33 +6 ☔ View full report in Codecov by Sentry. |
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.
@irfanuddinahmad This PR has already merged and been released to production, but it's causing several style regressions throughout the application due to the SCSS added within this PR.
I would recommend applying a fix-forward to ensure:
- Skills Quiz v2 styles "namespaced" under a skills quiz v2 specific class name to avoid style regressions on unintended areas of the MFE.
- Skills Quiz v2 styles imported from top-level styles entry point to ensure all SCSS variables are available in custom styles without defining variables yourself.
- Hard coded HEX color values removed.
- Use CSS utility classes, where relevant, for spacing (i.e., margin, padding).
@@ -0,0 +1,102 @@ | |||
import { Helmet } from 'react-helmet'; | |||
import './styles/index.scss'; |
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] Current convention in this MFE is to import component stylesheets within the top-level src/index.scss
file as opposed to within the component's JS itself as done here.
Rationale: by importing through JS as done here, the SCSS cannot make use of the existing SCSS theme variables without importing duplicate variable definitions into this stylesheet first. By importing this SCSS file within src/index.scss
instead, the skills quiz v2 SCSS can make use of Paragon theme variables.
For example, the styles for the original skills quiz is imported within src/index.scss
here.
@@ -0,0 +1,37 @@ | |||
$gray-500: #707070; |
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.
This is already the default color for both the Open edX and edX.org themes... No need to define it. See other comment regarding including the skills quiz v2 styles in src/index.scss
to get around needing to re-define SCSS variables that technically already exist.
.btn { | ||
margin: 7px -5px; | ||
} |
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, any class names overridden in component-specific SCSS files may also apply globally across the entire application... as is, the overridden margin on the .btn
class here is impacting all buttons across the entire application.
As a result, overriding these global class names like .btn
, .form
, .pgn__form-label
, etc. is causing style regressions wherever they are used outside of skills quiz v2, e.g. extra margin on the "Enroll" button the course page.
Also note, when dealing with spacing values (margin/padding), its generally best to use CSS utility classes (e.g., my-2 mx-1
) to avoid needing to write custom SCSS and to ensure consistency with other spacing values (e.g., 7px
and 5px
are not acceptable spacing values in the Paragon design system (docs).
Related, it's generally a code smell if you're modifying core styles like this... we should be using as much of default Paragon and @edx/brand-edx.org
as possible; otherwise, we're introducing style inconsistencies across the application/platform.
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.
To get around the issue of having styles apply globally across the entire MFE application is to nest generic style overrides in a class name that is specific to the feature/page route/etc. you're hoping the styles should apply to.
For example, add a class name to the top-most element of skills quiz v2 with a class name of skills-quiz-v2
and then have all these custom styles nested under the .skills-quiz-v2
class name so that the generic styles don't impact anything outside of .skills-quiz-v2
.
@@ -0,0 +1,43 @@ | |||
$dark-500: #00262b !default; |
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.
This color value is specific to @edx/brand-edx.org
and should not be committed; in fact, any hardcoded hex values in an MFE is generally a code smell as we should be relying on the style properties defined by the design system itself (CSS utility classes, SCSS variables), which ensures the MFE remains themeable.
https://paragon-openedx.netlify.app/foundations/colors/
In this case, would recommend using either the existing $dark-500
variable (no need to re-define it...) or relying on the appropriate CSS utility class instead.
.card { | ||
margin-right: 10px; | ||
min-height: 10em; | ||
} |
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.
@@ -0,0 +1,25 @@ | |||
$accent-b: #f0cc00 !default; |
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.
Another hardcoded hex value; should use already-defined $accent-b
.
Jira Ticket: https://2u-internal.atlassian.net/browse/ENT-7367
Description: Integrated the new dropdown design with Algolia, enabling it to display selected options and retrieve results from Algolia upon their selection.
UI Screenshots:
For all changes
Only if submitting a visual change