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

Fixes the grade removing issues in studio. #39

Closed

Conversation

farhaanbukhsh
Copy link
Member

@farhaanbukhsh farhaanbukhsh commented Oct 10, 2024

Description

Recently we started noticing that the grade page in the studio doesn't have a way to remove grades once you add them.
This is not the default behavior and it happens only when we add custom themes to the platform.

The default behavior is:

grading.mp4

What are we observing:

grading_glitch.mp4

The fix:

grading_fix.mp4

Useful information to include:

  • We should add in the document/Read me of how one can make fixes and what is the right place to put what kind of information.

Supporting information

Private-ref: BB-9232
open-craft/frontend-app-learning#27

Testing instructions

The instruction can get a bit complicated so I am trying to simplify it as much as possible.

I am trying to test course-authoring-mfe here since that is the mfe which is related to studio.

If you have tutor configured and running redwood:

  1. Go to studio
  2. Select any course
  3. Go to "Setting" --> "Grading" (You will find it in header)
  4. Try adding grades
  5. Hover over the grades in between and you will see a "Remove" button

This will prove that the functionality is working in the platform. Let's configure runtime themeing and see what happens.

  1. tutor mounts add /frontend-app-course-authoring
  2. tutor images build openedx-dev
  3. Once that is done cd into frontend-app-course-authoring and change the branch to asu-moe/redwood-css since the token support is available in that branch.
  4. We need to edit MFE_CONFIG I have https://github.com/farhaanbukhsh/tutor-contrib-expermental-mfe to test config, clone this and install in the virtual env of tutor.
  5. Enable the plugin 'tutor plugins enable expermental-mfe'
  6. tutor dev restart lms cms
  7. Once this is done and if you open studio you should see broken UI.
  8. Now clone https://github.com/open-craft/edx-simple-theme and change the branch to [artur/design-tokens-update](https://github.com/open-craft/edx-simple-theme/tree/artur/design-tokens-update)
  9. Run npm ci and npm run build inside the repo
  10. cd dist and run python3 -m http.server 9100
  11. Now hard refresh the studio page and it should be working fine now.
  12. Try to add and remove grade as mentioned above and you shouldn't be able to.
  13. Change the branch in edx-simple-theme to farhaan/fix-grading-issue
  14. npm run build and restart the python server.
  15. You should be able to add and remove the grade.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

@xitij2000 can I get a second opinion here from you, I want to know if this is the right way to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

There are a lot of hard-coses values here. Could you use the appropriate variables instead?

Can't look deeper since I'm on leave and on mobile.

Copy link
Member

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@farhaanbukhsh The solution works perfectly as described in the instructions. However, I think we should put the solution directly into the app itself instead of the theme. As the issue seems to occur independent of the theme in the asu-moe/redwood-css branch, it would make more sense to fix it there directly.

W.r.t the point raised by @xitij2000 about hardcoded values, I see that the original values are hardcoded as well. Futhermore, the buttons are being absolute positioned, so theme vars might not yield the right solution.

@farhaanbukhsh
Copy link
Member Author

@tecoholic Thanks a lot for the review, that makes sense.
@xitij2000 thanks for the review :)

I am closing this PR in favour of open-craft/frontend-app-authoring#72

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