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

feat: Using Paragon CSS External Hosting (JavaScript-based configuration) #726

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

Conversation

PKulkoRaccoonGang
Copy link

@PKulkoRaccoonGang PKulkoRaccoonGang commented Jul 3, 2024

Description

The JavaScript-based configuration approach allows the user to add Paragon CSS from external hosting.

module.exports = {
  PARAGON_THEME_URLS: {
    core: {
      urls: {
        default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/core.min.css',
      },
    },
    defaults: {
      light: 'light',
    },
    variants: {
      light: {
        urls: {
          default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/light.min.css',
        },
      },
    },
  },
};

Info

Related PRs

refactor: code refactoring

feat: necessary dependencies added

refactor: corrected lint

refactor: corrected sh

refactor: added logs

refactor: added temp frontend-platform dist

refactor: updated run-build-for-gh-deps.sh

refactor: corrected run-build-for-gh-deps.sh

refactor: fixing the problem

refactor: changed run build command

feat: added header

refactor: added dark theme and footer

refactor: code refactoring

refactor: code refactoring

fix: fixed CommentsView button

feat: added brand import

feat: removed brand
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 3, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Jul 3, 2024

Thanks for the pull request, @PKulkoRaccoonGang!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/edx-infinity. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@PKulkoRaccoonGang PKulkoRaccoonGang added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Jul 3, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@PKulkoRaccoonGang PKulkoRaccoonGang changed the title feat: implementation of Paragon design tokens testing: Using Paragon CSS External Hosting (JavaScript-based configuration) Jul 3, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang self-assigned this Jul 3, 2024
@PKulkoRaccoonGang PKulkoRaccoonGang marked this pull request as draft July 3, 2024 17:21
@brian-smith-tcril
Copy link
Contributor

I assume this visual bug isn't related to the styles being delivered via CDN but I noticed it here so I figure documenting it is worth doing

Screenshot from 2024-07-05 11-18-13
Screenshot from 2024-07-05 11-18-08
Screenshot from 2024-07-05 11-18-03

@dcoa
Copy link

dcoa commented Jul 6, 2024

I assume this visual bug isn't related to the styles being delivered via CDN but I noticed it here so I figure documenting it is worth doing

Hi @brian-smith-tcril, it's because of the styles loaded by the dark theme if you change your device theme to light, the default paragon styles will be loaded

@dcoa
Copy link

dcoa commented Jul 6, 2024

Thank @PKulkoRaccoonGang, all the design token work is fantastic and looks good. 🥳

I noticed some changes that are not related to the design tokens but It's affecting the styles.

These lines (from PR #697) override font-size 14px and 16px affecting header and footer, not only the main area

This is how the dropdown is looking
image

This is how supported to be (redwood)
image

We can increase the specification of the rule to avoid affecting the header and footer, also we can use var(--pgn-typography-font-size-sm) instead of 14px

Also, this change made the submit button resize, which looks weird to me, I mean, the component usually has a weight resize when executing the summit (because the length of the text changes and an icon is added). However, due to the text reduction (but the icon maintains the original one) a height resize is added.

scrnli_7_6_2024_12-00-00.PM.webm

@dcoa
Copy link

dcoa commented Jul 6, 2024

This is again not directly related to design tokens implementation, I'm not 100% sure if it's related to Paragon (that's why I highlight it) or the discussions implementation but the tabs course menu is not displaying the "more" button for responsive support during the first load, I have to resize the screen to make it work.

scrnli_7_6_2024_12-46-06.PM.webm

And again the changes from PR #697 are affecting the "more" button and the dropdown text size

@PKulkoRaccoonGang
Copy link
Author

PKulkoRaccoonGang commented Jul 7, 2024

@brian-smith-tcril @dcoa thanks!

@brian-smith-tcril we have prepared Paragon design tokens, which MFE Discussions consumes as the openedx theme. I added the brand-edx.org theme as a dark theme to demonstrate how the theme-switching functionality works.
The design tokens located in the brand-edx-org were not prepared as part of the work of this scope. This might be a great thing to do in future activities! 💯

Is the brand-edx.org theme mandatory to release updated MFEs in Sumac?

This is again not directly related to design tokens implementation, I'm not 100% sure if it's related to Paragon (that's why I highlight it) or the discussions implementation but the tabs course menu is not displaying the "more" button for responsive support during the first load, I have to resize the screen to make it work.

For testing, I prepared an additional sandbox, which is based on the master branch of the current repository (without changes related to design tokens). Unfortunately, the problem you described relates to already existing changes that do not directly relate to design tokens. I suggest creating a corresponding issue in this repository.

We can increase the specification of the rule to avoid affecting the header and footer, also we can use var(--pgn-typography-font-size-sm) instead of 14px

Also, this change made the submit button resize, which looks weird to me, I mean, the component usually has a weight resize when executing the summit (because the length of the text changes and an icon is added). However, due to the text reduction (but the icon maintains the original one) a height resize is added.

I propose not to make changes/corrections related to the original styles of MFE Discussions. This approach will make it possible to provide for review only the diff that is associated with the adding functionality of the Paragon design tokens.
Let's create issues for these questions in this repository.

@dcoa @brian-smith-tcril what are your thoughts?

@PKulkoRaccoonGang PKulkoRaccoonGang changed the title testing: Using Paragon CSS External Hosting (JavaScript-based configuration) feat: Using Paragon CSS External Hosting (JavaScript-based configuration) Jul 8, 2024
@dcoa
Copy link

dcoa commented Jul 10, 2024

Thanks @PKulkoRaccoonGang, I agree with you.

In terms of the design tokens implementation, I didn't find an issue. All looks as expected 🥳.

@brian-smith-tcril
Copy link
Contributor

I assume this visual bug isn't related to the styles being delivered via CDN but I noticed it here so I figure documenting it is worth doing

Hi @brian-smith-tcril, it's because of the styles loaded by the dark theme if you change your device theme to light, the default paragon styles will be loaded

Ah, yes, I have my system default set to dark mode so that makes sense.

I'm curious as to how this would behave if we changed

    variants: {
      light: {
        urls: {
          default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/light.min.css',
        },
      },
      dark: {
        urls: {
          default: 'https://cdn.jsdelivr.net/npm/@edx/brand-edx.org@alpha/dist/light.min.css',
        },
      },
    },

to be

    variants: {
      light: {
        urls: {
          default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/light.min.css',
        },
      },
      dark: {
        urls: {
          default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/light.min.css',
        },
      },
    },

I'd like to ensure that people (like me!) that have dark mode as a system default don't experience broken themes. It's not a problem for a website to not have a dark theme, but it is a problem if prefers-color-scheme: dark causes UI bugs.

@adamstankiewicz
Copy link
Member

Is the brand-edx.org theme mandatory to release updated MFEs in Sumac?

Given @edx/brand-edx.org is owned by 2U/edX.org, the ownership around finishing the migration to design tokens should likely be with 2U/edX.org. That said, we do not currently have that work prioritized owned/scoped/triaged yet but as this gets closer, it'll become higher priority. FWIW, our current assumption (based on prior discussions) is that PWG plans to backport critical security/bug fixes/new capabilities to the current v22 release (at least for a period of time) while instances like 2U/edX.org adequately migrate to design tokens. Worth noting, there is also a chance some of our MFEs may migrate directly to a newly created @edx/elm-theme, which already depends on alpha's tokens cc @PKulkoRaccoonGang @brian-smith-tcril

@dcoa
Copy link

dcoa commented Jul 16, 2024

I'd like to ensure that people (like me!) that have dark mode as a system default don't experience broken themes. It's not a problem for a website to not have a dark theme, but it is a problem if prefers-color-scheme: dark causes UI bugs.

@brian-smith-tcril, dark theme is optional we can use a definition that is

module.exports = {
  PARAGON_THEME_URLS: {
    core: {
      urls: {
        default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/core.min.css',
      },
    },
    defaults: {
      light: 'light',
    },
    variants: {
      light: {
        urls: {
          default: 'https://cdn.jsdelivr.net/npm/@openedx/paragon@alpha/dist/light.min.css',
        },
     },
  },
};

@PKulkoRaccoonGang PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Jul 22, 2024
Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.88%. Comparing base (422fbf6) to head (b2a409b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #726   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files         160      160           
  Lines        3329     3329           
  Branches      888      898   +10     
=======================================
  Hits         3092     3092           
+ Misses        219      218    -1     
- Partials       18       19    +1     

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

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@PKulkoRaccoonGang PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Jul 22, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR open-source-contribution PR author is not from Axim or 2U
Projects
Status: Waiting on Author
Development

Successfully merging this pull request may close these issues.

6 participants