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: Add support for showing and hiding parts of the progress page #36

Open
wants to merge 2 commits into
base: opencraft-release/palm.1
Choose a base branch
from

Conversation

xitij2000
Copy link
Member

No description provided.

Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

@xitij2000, please see the following recording. We cannot have this kind of delay when hiding the elements on the page:
Screencast_20241008_182416.webm

I just noticed that there is already a model with a per-course Progress page configuration that's used by the Course Home API. If we would need to add artificial delays to rendering the interface until the Course Blocks API gives us the response, maybe extending this configuration would actually be better?

Also, a note about the Pages & Resources - the options should either:

  1. Default to True.
  2. Be "Hide the component" instead of "Show the component".

Otherwise, visiting the "Configure progress" modal and pressing "Save" changes the Progress page configuration, which is confusing.

@xitij2000
Copy link
Member Author

@Agrendalath Sure, I think it makes sense to hide by default. I did intend in the code to default an undefined value to true, but I guess that didn't work? The logic should be to hide till the value is known, but have an undefined value equate to true so that if this isn't configured, then the component is shown by default.

The issue isn't with extending that configuration, it's with extending it in a way that is upstream-friendly. Making a change to an existing model is asking for trouble during upgrades when we need to cherrypick migrations etc.

If upstream is okay with having this feature, then we can move this to the man course structure and return it with the existing API.

@Agrendalath
Copy link
Member

@xitij2000,

Sure, I think it makes sense to hide by default. I did intend in the code to default an undefined value to true, but I guess that didn't work? The logic should be to hide till the value is known, but have an undefined value equate to true so that if this isn't configured, then the component is shown by default.

What are the next steps here?

The issue isn't with extending that configuration, it's with extending it in a way that is upstream-friendly. Making a change to an existing model is asking for trouble during upgrades when we need to cherrypick migrations etc.

If needed, we can backport this change only for this client and handle it during the upgrade. It won't be upstream-unfriendly this way.

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.

2 participants