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

fix: add patches to compile and enable themes #5

Merged
merged 5 commits into from
Sep 6, 2024
Merged

Conversation

MaferMazu
Copy link
Collaborator

@MaferMazu MaferMazu commented Aug 29, 2024

Description

This PR adds the necessary patches to compile and enable the themes.

For adding the patches we read: TUTOR_VERSION, PICASSO_THEME_DIRS and PICASSO_THEMES_NAME, similar to tutor-contrib-edunext-distro.

Context

With the command enable-themes, we are cloning our repository themes in /openedx/themes, but the themes sometimes aren't in the root of that repository, and we need to specify the platform where to find our themes. So, we use PICASSO_THEME_DIRS. We also need to compile the themes and enable the comprehensive themes.

How to test it

This code is used in this job: https://picasso.dedalo.edunext.co/job/PicassoV2_and_picasso_tutor_plugin/

I have already tried it with the following:

Extra

Consequence: I propose changing TUTOR_VERSION to PICASSO_TUTOR_VERSION because we will need something to have different versions of the compile.

@MaferMazu MaferMazu marked this pull request as ready for review August 30, 2024 21:41
Copy link
Contributor

@bra-i-am bra-i-am left a comment

Choose a reason for hiding this comment

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

From my perspective, everything is fine.

I only think that the *-env patches are using PICASSO_DEFAULT_SITE_THEME so this property should be in the README too.

README.rst Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

I'll be testing this throughout the day. Thanks!

@MaferMazu
Copy link
Collaborator Author

MaferMazu commented Sep 5, 2024

Thanks for the feedback, @mariajgrimaldi @dcoa. I'd love a re-review.

Copy link
Contributor

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

LGTM!

@MaferMazu MaferMazu merged commit f16be61 into main Sep 6, 2024
7 checks passed
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.

4 participants