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: make enterprise modal blocking #550

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kiram15
Copy link
Contributor

@kiram15 kiram15 commented Jan 16, 2025

Verizon requested that the modal that attempts to direct learners to the enterprise dashboard have to be either accepted or dismissed instead of being able to click out of it.

Jira ticket

Screenshot 2025-01-15 at 11 24 21 PM

@kiram15 kiram15 requested a review from a team as a code owner January 16, 2025 06:25
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.35%. Comparing base (abae82b) to head (a77868f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #550   +/-   ##
=======================================
  Coverage   97.35%   97.35%           
=======================================
  Files         154      154           
  Lines        1362     1362           
  Branches      229      228    -1     
=======================================
  Hits         1326     1326           
  Misses         34       34           
  Partials        2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@deborahgu deborahgu left a comment

Choose a reason for hiding this comment

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

looking at this now

@deborahgu deborahgu added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jan 16, 2025
Copy link
Member

@deborahgu deborahgu left a comment

Choose a reason for hiding this comment

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

so, I checked on this. The only changes we're supposed to be making to 2u-specific code in openedx repos is removing it. Can you make this into a plugin? The general guidance we've gotten from Axim is that it is nearly always okay to make a plug-in slot if adding the plug-in slot means we are removing 2u-specific functionality, so making a front end plug-in framework plugin for this would be the right approach.

They are pretty straightforward and wicked fast to make, and you can see prior art and an excellent README in https://github.com/edx/frontend-plugin-learner-dashboard. If you wanted to you could use that repo, or you could make one specifically for enterprise/B2B were you wouldn't have to ask our opinions about things at all, 😁.

You can see how they are configured and run by looking at prior art in the YAML and JS config files in https://github.com/edx/edx-internal/tree/master/frontends/frontend-app-learner-dashboard.

we are happy to pair with you on doing this if you want help making it happen faster!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for eng review PR is ready for review. Review and merge it, or suggest changes.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants