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] feat: added support Paragon design tokens #351

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

PKulkoRaccoonGang
Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang commented Jun 15, 2023

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Jun 15, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jun 15, 2023

Thanks for the pull request, @PKulkoRaccoonGang!

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

@adamstankiewicz adamstankiewicz changed the title [BD-46] feat: added support Paragon design tokens [BD-46] feat!: added support Paragon design tokens Jul 14, 2023
Comment on lines 1 to 4
@import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/core.min.css");
@import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/light.min.css");
@import "@edx/paragon/styles/scss/core/core";
@import "@edx/paragon/styles/css/themes/light/index.css";
Copy link
Member

Choose a reason for hiding this comment

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

[clarification] As is, I believe these would be importing duplicate styles? For example, @import "@edx/paragon/styles/css/themes/light/index.css"; is largely the same import as @import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/light.min.css"); above.

If we're importing the core.css and light.css files via example/index.scss, we should likely rely on the local imports from @edx/paragon. Take a look at the draft migration documentation for an example of what I imagine MFEs would need to do to import the compiled Paragon CSS locally: https://openedx.atlassian.net/wiki/spaces/BPL/pages/3770744958/Migrating+MFEs+to+Paragon+design+tokens+and+CSS+variables#Using-locally-installed-Paragon-CSS

Alternatively, we could also consider configuring the example MFE to utilize the @edx/frontend-platform approach to injecting the Paragon CSS from external CDN url like jsDelivr once openedx/frontend-platform#440 merges.

src/index.scss Outdated
Comment on lines 1 to 4
@import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/core.min.css");
@import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/light.min.css");
@import "@edx/paragon/styles/scss/core/core";
@import "@edx/paragon/styles/css/themes/light/index.css";
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 we should consider whether there are any performance concerns around coupling Paragon's styles with these shared components. For example, if both these component includes all of Paragon's CSS and the consuming MFE includes all of Paragon's CSS, will the user essentially need to download all of Paragon's CSS twice?

I'm wondering if it would make sense for the shared components to be de-coupled from Paragon styles, with the assumption that these components will always be used within an application that provides Paragon styles externally from the component. A potential concern with this proposed approach may be around versioning; that is, is there a risk that developers could run into version discrepancies between what this component expects/uses and what version of Paragon's CSS is provided by the consuming/hosting MFE?

If we do leave Paragon styles coupled with the shared component, it may be a good to exercise to quantify/benchmark the performance implications of both approaches?

What do you think?

Choose a reason for hiding this comment

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

Personally, I feel like we should de-couple Paragon styles from these components. The biggest problem with leaving Paragon's styles here is that we're dealing with CSS now - if we import Paragon's CSS variables here, then they will override all CSS variables from MFE's Paragon and this will affect the whole MFE (it will happen because header / footer styles are usually imported last in the MFE). This will basically mean that header component is in charge of the theme for the whole platform.

And even if we switch import order in the MFE, it will just mean that MFE will override all CSS used by header component.

I think if we're migrating to CSS, then MFEs should be a single source of truth for the styles in the platform. People should also understand that they cannot override CSS variables in their styles because that will likely affect this variable across all styles.

As for the problem with version discrepancies... The first thing we could do is to advise engineers to only use global tokens from Paragon in SCSS files in header / footer components as they are less likely to change in Paragon. But the main problem is with components I guess, because they use CSS classes from Paragon which are a lot likely to change between versions, not sure what to do here. The only long-term solution that comes to mind is to de-couple header / footer components even further from Paragon and just do not use Paragon's component's here at all. And create new components that will re-use only Paragon's CSS variables so that Paragon would still control the theme of the header / footer.

Another idea is to move header and footer components into Paragon so they always share the version? I might be crazy though haha

@import "@edx/brand/paragon/overrides";
@import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/core.min.css");
@import url("https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/light.min.css");
@import "@edx/paragon/styles/scss/core/core";
Copy link
Member

Choose a reason for hiding this comment

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

Should this be importing the core.css rather than SCSS?

package.json Outdated
@@ -55,7 +55,7 @@
"redux-saga": "1.2.3"
},
"dependencies": {
"@edx/paragon": "20.44.0",
"@edx/paragon": "21.0.0-alpha.38",
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving this to a peer dependency.

@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as ready for review August 9, 2023 13:11
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/support-design-tokens branch from 180a7fb to c264b1b Compare August 9, 2023 13:27
@PKulkoRaccoonGang PKulkoRaccoonGang changed the title [BD-46] feat!: added support Paragon design tokens [BD-46] feat: added support Paragon design tokens Aug 9, 2023
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/support-design-tokens branch from c264b1b to 8a7607e Compare August 9, 2023 13:33
@PKulkoRaccoonGang PKulkoRaccoonGang force-pushed the Peter_Kulko/support-design-tokens branch from 8a7607e to dc84f97 Compare August 18, 2023 10:52
ghassanmas added a commit to ghassanmas/frontend-app-learning that referenced this pull request Oct 6, 2023
  This PR purpose is to test/demo parago design tokens simliar
  to this one for the profile openedx/frontend-app-profile/pull/764

  it override the following depns as seen in package.json

  - paragon alpha
  - openedx/frontend-build/pull/365
  - openedx/frontend-platform/pull/440
  - openedx/frontend-component-header/pull/351
  - openedx/frontend-component-footer/pull/303

 Conclousion so far:

 - There is an extra library that learning depends on which
  needs to support paragon; `frontend-lib-learning-assistant`
  and also `frontend-lib-special-exams`

 - It would be great to have a gudie or a document to look at,
 while doing the conversion that would **map variable from
 who it was used/named before to the name in design tokens**

 - I was stuck in the end with compliation error, that
 wepack couldn't find `Modal` exported from paragon.
@Mashal-m
Copy link
Contributor

Hey @PKulkoRaccoonGang, What is the current status of this PR, is it ready to review and merge?
Could you please resolve conflicts?

@PKulkoRaccoonGang
Copy link
Contributor Author

PKulkoRaccoonGang commented Nov 17, 2023

@Mashal-m Hello! At the moment, all pull requests opened in the edx and openex repositories are awaiting further action on the use of Paragon design tokens in the MFE. Don't pay any attention to them for now.

Full list of pull requests that should just be open for now:

@dcoa
Copy link

dcoa commented Jun 5, 2024

Hi @PKulkoRaccoonGang, thanks for this. I checked these changes and it is still relevant.

Here we should rebase the branch to include the StudioHeader.scss

.studio-header {
  position: relative;
  z-index: 1000;

  height: 3.75rem;
  box-shadow: 0 1px 0 0 rgb(0 0 0 / .1);
  background: var(--pgn-color-white);

  .btn-outline-primary {
    border-color: var(--pgn-color-white);
  }

  .logo {
    display: block;
    box-sizing: content-box;
    position: relative;
    top: -.05em;
    height: 1.75rem;
    padding: var(--pgn-spacing-spacer-base) 0;
    margin-right: var(--pgn-spacing-spacer-base);

    img {
      display: block;
      height: 100%;
    }
  }

  .course-title-lockup {
    @media only screen and (min-width: 769px) {
      padding: .5rem;
      padding-right: var(--pgn-spacing-spacer-base);
      border-right: 1px solid #E5E5E5;
      width: 70%;
    }
  
    overflow: hidden;
  
    span {
      color: var(--pgn-color-gray-800);
      white-space: nowrap;
      overflow: hidden;
      text-overflow: ellipsis;
      line-height: 1.375rem;
    }
  }
}

src/index.scss Outdated
}
}

.content b {
Copy link

Choose a reason for hiding this comment

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

[Question] I tested without the styles below and the UI remained the same.

I couldn't find a reference to these styles, do you think are still relevant or can we get rid of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that if there are no visual defects, then these styles are not needed)

@import "@edx/brand/paragon/variables";
@import "@edx/paragon/scss/core/core";
@import "@edx/brand/paragon/overrides";

@import "@edx/frontend-component-header/index";
Copy link

Choose a reason for hiding this comment

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

Suggested change
@import "@edx/frontend-component-header/index";
@use "@openedx/paragon/dist/core.min.css" as paragonCore;
@use "@openedx/paragon/dist/light.min.css" as paragonLight;
@import "@edx/frontend-component-header/index";

For the example app, I think we can use the Paragon installed versions, in this way, it's not necessary to add a js config file, and the styles are applied only for the example app.

@dcoa
Copy link

dcoa commented Jun 5, 2024

Also with the changes in frontend-build the webpack.dev.config.js needs a change for the example app, the change is only in the entry key:

  entry: {
    app: path.resolve(__dirname, 'example'),
  },

@dcoa dcoa force-pushed the Peter_Kulko/support-design-tokens branch 2 times, most recently from cfca879 to 1580c3c Compare June 5, 2024 10:44
@dcoa dcoa force-pushed the Peter_Kulko/support-design-tokens branch from 93f95e5 to d2bf693 Compare June 25, 2024 21:44
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
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants