feat: ENT-7367 Added skills quiz v2 #911
Conversation
irfanuddinahmad
force-pushed
the
iahmad/ENT-7367-2
branch
2 times, most recently
from
45b4217
to
5b65a77
Compare
mahamakifdar19
force-pushed
the
iahmad/ENT-7367-2
branch
4 times, most recently
from
79a3f80
to
5a79d78
Compare
mahamakifdar19
force-pushed
the
iahmad/ENT-7367-2
branch
from
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.
@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.
[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.
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.
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.
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.
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.
This impacts all .card classnames throughout the entire MFE application, causing unintended but noticeable style regressions, e.g. adding too much height beneath the "Find a course" CTA on the dashboard page route's subsidies summary sidebar box as shown below:
| @@ -0,0 +1,25 @@ | |||
| $accent-b: #f0cc00 !default; | |||
There was a problem hiding this comment.
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