search

openedx / frontend-app-discussions

A React-based micro frontend for the Open edX discussion forums.
GNU Affero General Public License v3.0
6 stars 64 forks source link

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

Open PKulkoRaccoonGang opened 4 months ago

PKulkoRaccoonGang commented 4 months ago

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

openedx-webhooks commented 4 months ago

Thanks for the pull request, @PKulkoRaccoonGang!

What's next?

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

:radio_button: Get product approval

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

:radio_button: 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:

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

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

:radio_button: 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:

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

open-craft-grove commented 4 months ago

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

brian-smith-tcril commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 https://github.com/openedx/frontend-app-discussions/pull/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 commented 4 months ago

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 https://github.com/openedx/frontend-app-discussions/pull/697 are affecting the "more" button and the dropdown text size

PKulkoRaccoonGang commented 4 months ago

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

dcoa commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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',
        },
     },
  },
};
codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.88%. Comparing base (422fbf6) to head (b2a409b). Report is 8 commits behind head on master.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

open-craft-grove commented 4 months ago

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

open-craft-grove commented 4 months ago

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

open-craft-grove commented 4 months ago

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

mphilbrick211 commented 3 weeks ago

@PKulkoRaccoonGang - can this be closed, or is it still in progress?

brian-smith-tcril commented 3 weeks ago

@mphilbrick211 this PR should stay open. The plan is to land it in a new long living branch for design tokens once it is updated to use a published version of Paragon that supports design tokens (which is getting really close!)