Skip to content

fix: rewrite calcualtor tips to reflect the supported features#29957

Merged
rgraber merged 2 commits intoopenedx:masterfrom
ghassanmas:fix-calc-tips
Mar 10, 2022
Merged

fix: rewrite calcualtor tips to reflect the supported features#29957
rgraber merged 2 commits intoopenedx:masterfrom
ghassanmas:fix-calc-tips

Conversation

@ghassanmas
Copy link
Member

@ghassanmas ghassanmas commented Feb 21, 2022

Description and Related information

It replicates this PR: openedx/frontend-app-learning/pull/825 which
rewrite calcualtor tips since some tips are no longer relevant,
hence: #17368

Testing:

To be to test this change, here are the steps:

  • Make sure there is a course with Show Calcutrator to true, follow this doc link for more details
  • Go to the learning experience as shown (from the above link as well)**
  • When clicking on i button the calculator instructions tips, should reflect the change of this PR. e.g., there should be no example of using T as constant...etc.

** You would need to make sure that you are interacting with the course through the old course experience, not using the learning MFE hence, it's already resolved for the learning MFE openedx/frontend-app-learning/pull/825.
Relative information about how enable use old courseware experience:

The courseware.use_legacy_frontend and course_home.course_home_use_legacy_frontend Waffle flags can be toggled on (either globally or per-course-run) in order to revert to the Legacy (LMS Django-rendered) courseware experience.
ref

@openedx-webhooks
Copy link

Thanks for the pull request, @ghassanmas! I've created OSPR-6481 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Feb 21, 2022
@ghassanmas
Copy link
Member Author

This ready for review also this #29080 as well

@mariajgrimaldi
Copy link
Member

Hi there @ghassanmas!

Can you add testing instructions to your cover letter? That would be helpful for others! 😄

@ghassanmas
Copy link
Member Author

@mariajgrimaldi Just did wrote detailed instructions to test and provided more context as well. Let me know if you have any questions.

@natabene
Copy link
Contributor

natabene commented Mar 2, 2022

@ghassanmas Thank you for the contribution. I will line this up for our review.

@rgraber
Copy link
Contributor

rgraber commented Mar 7, 2022

@ghassanmas it seems like quality checks are failing, please resolve and then I can rereview

  - It replicate this PR: frontend-app-learning/pull/825 which
  rewrite calcualtor tips since some tips are no longer relevant,
  hence: openedx#17368
@ghassanmas
Copy link
Member Author

@rgraber I did ran the ./scripts/xss-commit-linter.sh locally and got 0 violation. But how am I suppose to fix it here? Am I suppose to push result file humm

@rgraber
Copy link
Contributor

rgraber commented Mar 8, 2022

@rgraber I did ran the ./scripts/xss-commit-linter.sh locally and got 0 violation. But how am I suppose to fix it here? Am I suppose to push result file humm

I'll take a look once tests run again

@rgraber
Copy link
Contributor

rgraber commented Mar 8, 2022

Apparently there is a known issue with this test. We are going to disable it since it's not really that valuable anymore. at that point i'll rerun the tests on this guy and hopefully that will be enough

@ghassanmas
Copy link
Member Author

Alright, in other words the tests needs tests 😅

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

LGTM

@rgraber rgraber merged commit 62d1ebf into openedx:master Mar 10, 2022
@openedx-webhooks
Copy link

@ghassanmas 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments