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.

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

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

Merged

Conversation

PKulkoRaccoonGang
Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang commented Jul 5, 2023

Description

Fixed:

Issues:

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 openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 5, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jul 5, 2023

Thanks for the pull request, @PKulkoRaccoonGang!

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

@netlify
Copy link

netlify bot commented Jul 5, 2023

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...

QR Code

Use your smartphone camera to open QR code link.

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

@PKulkoRaccoonGang PKulkoRaccoonGang changed the title refactor: fixed spacing calculations and box-shadows [BD-46] refactor: fixed spacing calculations and box-shadows Jul 5, 2023
@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Jul 5, 2023
@PKulkoRaccoonGang PKulkoRaccoonGang self-assigned this Jul 5, 2023
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (09570d7) 91.95% compared to head (daacf73) 91.95%.

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.

@@ -320,7 +320,7 @@ $escaped-characters: (

$enable-caret: true !default;
$enable-rounded: true !default;
$enable-shadows: false !default;
$enable-shadows: true !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

$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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as ready for review September 8, 2023 12:32
@viktorrusakov viktorrusakov merged commit 948e51e into openedx:alpha Sep 15, 2023
9 checks passed
@openedx-webhooks
Copy link

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

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

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

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

The release is available on:

Your semantic-release bot 📦🚀

PKulkoRaccoonGang added a commit that referenced this pull request Aug 1, 2024
* refactor: fixed spacing calculations and box-shadows

* refactor: removed flag to switch shadows
PKulkoRaccoonGang added a commit that referenced this pull request Aug 4, 2024
* refactor: fixed spacing calculations and box-shadows

* refactor: removed flag to switch shadows
PKulkoRaccoonGang added a commit that referenced this pull request Aug 4, 2024
* refactor: fixed spacing calculations and box-shadows

* refactor: removed flag to switch shadows
PKulkoRaccoonGang added a commit that referenced this pull request Aug 5, 2024
* refactor: fixed spacing calculations and box-shadows

* refactor: removed flag to switch shadows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program released on @alpha
Projects
Archived in project
Status: Done
7 participants