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

chore: Update to the new version of paragon in the new scope. #1930

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Oct 30, 2023

Part of openedx/axim-engineering#23

This replaces the @edx/paragon packag to point to the paragon package at
the openedx scope(@openedx/paragon). Imports have been updated to use the
same locations in the new package.

@feanil feanil marked this pull request as ready for review October 31, 2023 11:51
@feanil feanil requested a review from a team October 31, 2023 11:51
@feanil feanil force-pushed the feanil/update_paragon_dependency branch 2 times, most recently from 9ac928d to 1098271 Compare November 6, 2023 19:15
@feanil
Copy link
Contributor Author

feanil commented Nov 6, 2023

@openedx/enterprise-titans this is a fairly major version bump in the paragon version. Let me know if there are any specific tests that I should perform above the basic CI to verify this changes.

@adamstankiewicz
Copy link
Member

@openedx/enterprise-titans this is a fairly major version bump in the paragon version. Let me know if there are any specific tests that I should perform above the basic CI to verify this changes.

@feanil Luckily no React components from Paragon are used within this repo. That said, there are some features like the https://courses.edx.org/enterprise/select/active page that rely on some of the class names from Paragon/Bootstrap (e.g., .form-control) on the default Django UI elements. For example, this "Select an organization" page relies on the core Paragon styles and the aforementioned .form-control class name:

image

The changes made in this PR that first introduced @edx/paragon into this repo (in favor of @edx/edx-bootstrap) should have largely taken care of the majority of issues that faced the repo related to Paragon; the core class names like .form-control, typography, etc. from Bootstrap have not changed since the v12 version currently installed.

Given this, these changes otherwise feels fairly low risk to me, but some flows like the above one should likely be verified on stage/prod with a quick run through to ensure no style regressions. I'm not sure how familiar you are with setting up / testing Enterprise flows, so I'd be happy to help you with a quick manual run through of some common Enterprise user flows that depend on edx-enterprise like the one above once this lands on stage to ensure no style regressions.

@feanil
Copy link
Contributor Author

feanil commented Nov 7, 2023

@adamstankiewicz I don't have access to your stage systems to verify these changes there. Is it possible to set these up in a pre-stage environment like devstack or tutor?

@feanil
Copy link
Contributor Author

feanil commented Dec 12, 2023

@adamstankiewicz any update on this, do you have docs on how to setup the relevant views in a dev environment or can you do the verification on stage for this?

Part of openedx/axim-engineering#23

This replaces the `@edx/paragon` packag to point to the `paragon` package at
the `openedx` scope(`@openedx/paragon`). Imports have been updated to use the
same locations in the new package.
@feanil feanil force-pushed the feanil/update_paragon_dependency branch from 1098271 to 19cecd3 Compare December 12, 2023 17:18
@feanil
Copy link
Contributor Author

feanil commented Dec 12, 2023

@adamstankiewicz I was able to get this running locally and verify that there weren't any paragon CSS related errors on the enterprise pages:

Before upgrade

enterprise-before-paragon-upgrade

After Upgrade

enterprise-after-paragon-upgrade

I think this change is ready to go.

Copy link

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Approving based on @adamstankiewicz's comments and @feanil's testing.

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

@feanil thanks for verifying with the screenshot.

If this intends to be released to PyPi as a new version such that it can be upgraded in edx-platform, the PR should include a CHANGELOG entry and an update to the version specified in the init.py file.

@feanil @arbrandes Do you plan on getting these changes upgraded into edx-platform or are you planning on just merging the PR?

@feanil
Copy link
Contributor Author

feanil commented Dec 13, 2023

@adamstankiewicz yes, I'll bump the version, add the changelog entry and get this landed in edx-platform.

Update the changelog and bump the version.
@feanil feanil merged commit 56e2d21 into master Dec 13, 2023
7 checks passed
@feanil feanil deleted the feanil/update_paragon_dependency branch December 13, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants