[BD-46] refactor: fixed spacing calculations and box-shadows

[PKulkoRaccoonGang]

Description

Fixed:

• Spacing calculation errors
• Box-shadows

Issues:

• [alpha] Dropdown box-shadow #2390
• [alpha] Active button states #2389
• [alpha] SelectMenu incorrect color on hover #2392

Relative PR: Brand EdxOrg

Merge Checklist

• If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
• Does your change adhere to the documented style conventions?
• Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
• Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
• Were your changes tested in the example app?
• Is there adequate test coverage for your changes?
• Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

• Verify your changes were released to NPM at the expected version.
• If you'd like, share your contribution in #show-and-tell.
• 🎉 🙌 Celebrate! Thanks for your contribution.

[openedx-webhooks]

Thanks for the pull request, @PKulkoRaccoonGang!

When this pull request is ready, tag your edX technical lead.

[netlify]

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	daacf73
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/64fb14071769a200089d70cb
😎 Deploy Preview	https://deploy-preview-2423--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.95%. Comparing base (09570d7) to head (daacf73).
Report is 746 commits behind head on alpha.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #2423   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files         216      216           
  Lines        3618     3618           
  Branches      891      891           
=======================================
  Hits         3327     3327           
  Misses        286      286           
  Partials        5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[viktorrusakov]

why did you change it to true? it's false in the master branch

[PKulkoRaccoonGang]

For some reason, we have a problem with the display of shadows generated by the Bootstrap tool.
I tried to include the flag above and earned)

[viktorrusakov]

Oh I see what the issue is 😔. The problem with your solution is that this flag also enables box shadows for openedx theme which we shouldn't enable (because they are turned off in master branch). This flag is currently turned on in edX theme here, which overrides the flag in Paragon and allows to display shadows in edX theme.

However, this flag does not work in alpha branch because we don't override SCSS variables anymore since we use CSS variables... And I don't think there is way to make these global flags work with CSS variables.

I think the only solution here is to deprecate this flag and remove usage of box-shadow mixin from our codebase because it relies on this flag. Instead, box-shadows should only be controlled by CSS variables, e.g. for Dropdown component we should replace this line

paragon/src/Dropdown/dropdown-bootstrap.scss

Line 33 in cc022bb

@include box-shadow(var(--pgn-elevation-dropdown-box-shadow));

with just

box-shadow: var(--pgn-elevation-dropdown-box-shadow);

and set --pgn-elevation-dropdown-box-shadow variable to be none in Paragon but to be actual box-shadow value in edX brand theme.

We'll probably need to do the same for all other global SCSS flags...

@adamstankiewicz curious what you think here 🙂

[PKulkoRaccoonGang]

Yeah, it's one of the possible solutions. Looks interesting, thanks 💯

[adamstankiewicz]

I think the only solution here is to deprecate this flag and remove usage of box-shadow mixin from our codebase because it relies on this flag. Instead, box-shadows should only be controlled by CSS variables

I agree with what @viktorrusakov has mentioned, where the Dropdown box shadow for openedx theme should be none but for edX.org, it should be the actual value.

[clarification] Regarding deprecation, is there a way for us to legit deprecate these flags (i.e., have them still function as expected but warn that they will be soon removed). If we can't support the flag's functionality for consumers as is, then that feels like it is a breaking change that we may need to introduce now instead of a true deprecation? cc @viktorrusakov

[suggestion/question] When replacing the usage of the SCSS mixin with the CSS variable (e.g., box-shadow: var(--pgn-elevation-dropdown-box-shadow);), I might also consider if/when it makes sense to use the pgn-box-shadow mixin in the internal Paragon code. Do we intend to keep that pgn-box-shadow SCSS mixin around for consumers to use with design tokens?

We'll probably need to do the same for all other global SCSS flags...

@PKulkoRaccoonGang @monteri @viktorrusakov Do we have a good understanding of what all the other global SCSS flags are if we are considering deprecating/removing them, too?

[viktorrusakov]

Do we have a good understanding of what all the other global SCSS flags are if we are considering deprecating/removing them, too?

I don't think there should be any global SCSS flags anymore, SCSS shouldn't be able to control styles like this since design tokens are the single source of truth for styles now. We should consider migrating some flags to the design tokens schema I think (e.g. to be able to disable all box shadows by just changing one global token).

I think these are the only ones we have to take care for, I don't remember seeing any other flags, but we'll have to make sure of that also.

paragon/scss/core/_variables.scss

Lines 321 to 333 in 932eed0

	$enable-caret: true !default;
	$enable-rounded: true !default;
	$enable-shadows: false !default;
	$enable-gradients: false !default;
	$enable-transitions: true !default;
	$enable-prefers-reduced-motion-media-query: true !default;
	$enable-hover-media-query: false !default; // Deprecated, no longer affects any compiled CSS
	$enable-grid-classes: true !default;
	$enable-pointer-cursor-for-buttons: true !default;
	$enable-print-styles: true !default;
	$enable-responsive-font-sizes: false !default;
	$enable-validation-icons: false !default;
	$enable-deprecation-messages: true !default;

For what it's worth I don't think consumers use these flags much 😅. For example edX.org brand overrides only the shadows flag. Removing them would still be a breaking change for sure, I'm just saying that I don't think it's that big of a breaking change 🙂

Do we intend to keep that pgn-box-shadow SCSS mixin around for consumers to use with design tokens?

I think it's ok to keep them for now just to minimize amount of breaking changes. And I think they might still be useful to engineers so they could just write @include pgn-box-shadow(1, "down") instead of looking up CSS variable for that. I would probably suggest we change internal implementation of the mixin so it uses CSS variables directly instead of SCSS ones.

Regarding deprecation, is there a way for us to legit deprecate these flags (i.e., have them still function as expected but warn that they will be soon removed).

We could do that, but the problem is that it's an SCSS flag, which means that if consumers want to keep using them, they need to do that through SCSS, i.e. keep importing variables.scss file from their brand package into MFEs. They won't be able to override these flags just by including CSS files into the HTML head. That's actually why this flag doesn't work in alpha branch - we no longer import SCSS files from edX.org brand in docs site, so this flag is not overridden anymore.

Apart from this I don't see any problems with deprecating these flags just by overriding Bootstrap's / Paragon's mixins with inclusion of deprecation message there (and maybe other minor tweaks).

[PKulkoRaccoonGang]

@adamstankiewicz @viktorrusakov thanks for the suggestions. Please look at the new changes! 👀

[monteri]

There is only difference in variable is $enable-box-shadows that is true in the brand-edx.org. On one of the meeting we decided to remove variables completely BUT there are some dependencies on the React-Bootstrap side. So my current suggestion is that we will leave scss flags as is in the Paragon side so that styles relying on the React-Bootstrap would not break.

[PKulkoRaccoonGang]

@adamstankiewicz Regarding the rest of the Bootstrap flags that we have in Paragon (except for $enable-box-shadows). At the moment, in order to completely transfer all flags to design tokens, we have several problems:

• SCSS code blocks that are controlled by conditions (to solve this problem, may need to create multiple design tokens for each property)
• transferring Bootstrap files to the local bootstrap-override folder. This decision is inevitable because in the Paragon we are based on the SCSS of moisture that Bootstrap provides and for control from a “single source of truth” - the design of tokens, it is necessary to have full access to all Bootstrap files that use conditions that depend on the SCSS flags.

Conclusion and proposals: Having discussed with @monteri and @viktorrusakov, we are inclined to believe that we need to abandon the partial use of Bootstrap and transfer it completely locally to the project.
This solution will open up the possibility for us to fully optimize styles for CSS variables and design tokens. Will provide a chance to refactor existing styles and remove unnecessary ones.

[viktorrusakov]

This is not a blocker for this PR, so I'm going to merge it. We'll handle other SCSS flags in separate PRs.

[openedx-webhooks]

@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-semantic-release]

🎉 This PR is included in version 22.0.0-alpha.3 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[openedx-semantic-release-bot]

🎉 This PR is included in version 23.0.0-alpha.1 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[openedx-semantic-release-bot]

🎉 This PR is included in version 23.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀