-
Notifications
You must be signed in to change notification settings - Fork 108
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: adding a new plugin for the enterprise modal on dashboard #565
base: master
Are you sure you want to change the base?
Conversation
@kiram15, could we rename this plugin slot to be more generally applicable to the open source project? For example, it seems like this would be able to hold any initial modal, and isn't specific to enterprise. Is that right? It's just that we are using this slot for a modal that appears on first load to be the redirector for the Enterprise dashboard. |
ec8e31e
to
91b2e77
Compare
src/plugin-slots/DashboardModalSlot/images/dashboardModalExample.png
Outdated
Show resolved
Hide resolved
08abe31
to
03aeeff
Compare
03aeeff
to
9f57363
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
+ Coverage 97.36% 97.44% +0.08%
==========================================
Files 155 152 -3
Lines 1365 1330 -35
Branches 228 225 -3
==========================================
- Hits 1329 1296 -33
+ Misses 34 33 -1
+ Partials 2 1 -1 ☔ View full report in Codecov by Sentry. |
f630bf3
to
84fcd32
Compare
<DashboardModalSlot /> | ||
{(hasCourses && showSelectSessionModal) && <SelectSessionModal />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[consideration/thought question] Would it make sense to co-locate SelectSessionModal
as the default modal within DashboardModalSlot
? It could be a bit confusing to have default modals outside of DashboardModalSlot
(e.g., when should a default dashboard modal be put in this modal-specific slot vs. outside the modal-specific slot?).
If intentional to keep default dashboard modals out of the DashboardModalSlot
, perhaps DashboardModalSlot
is renamed to DashboardCustomModalSlot
to be explicit that its for custom modals, not default modals like SelectSessionModal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking it'd make sense to have the SelectSessionModal
as the default content as well, and it received some agreement during the most recent Frontend Working Group meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we have a customer waiting upon this change, I think this would be a good idea to accomplish as a fast-follow
bc580d9
to
4a8814f
Compare
2791d45
to
528fb79
Compare
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.
This is part of 3 different PRs that extract the enterprise modal into a plugin.
https://github.com/edx/edx-internal/pull/12301
https://github.com/edx/frontend-plugin-learner-dashboard/pull/58
Jira Ticket - ENT-9853