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

[ENT-9617] Show expiration modal on dashboard and search page #1206

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

mahamakifdar19
Copy link
Contributor

@mahamakifdar19 mahamakifdar19 commented Oct 11, 2024

Ticket
https://2u-internal.atlassian.net/browse/ENT-9617

Description
This PR makes the expiration modal available on all 3 primary sections (Dashboard, search, course about) of the learner portal on every visit after license expiration.

UI Screenshots
Search Page:
Screenshot 2024-10-11 at 7 00 40 PM

Dashboard
Screenshot 2024-10-11 at 7 00 20 PM

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)
  • Ensure English strings are marked for translation. See documentation for more details.

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.42%. Comparing base (5a1e7a6) to head (7b28c5b).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1206      +/-   ##
==========================================
+ Coverage   88.41%   88.42%   +0.01%     
==========================================
  Files         399      399              
  Lines        8502     8502              
  Branches     2094     2088       -6     
==========================================
+ Hits         7517     7518       +1     
  Misses        942      942              
+ Partials       43       42       -1     

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

@muhammad-ammar
Copy link
Contributor

@mahamakifdar19 The modal has a large empty space at the top, which looks unbalanced. Is this design approved by UX?

@mahamakifdar19
Copy link
Contributor Author

@mahamakifdar19 The modal has a large empty space at the top, which looks unbalanced. Is this design approved by UX?

@muhammad-ammar @jajjibhai008 is currently working on the updated design.

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

Left one more comment for consideration to remove some ambiguity/overlap between your new ExpiredSubscriptionModal and the existing SubscriptionExpirationModal, especially now that DashboardPage is responsible for rendering both. Without context, it may be unclear why there's two distinct components, both related to subscription expiration.

Your changes / the code itself look great; just a bit concerned about the ambiguity described above that I'd love your two cents on :)

@@ -57,6 +58,7 @@ const DashboardPage = () => {
<Container size="lg" className="py-4">
<Helmet title={PAGE_TITLE} />
<BudgetExpiryNotification />
<ExpiredSubscriptionModal />
Copy link
Member

Choose a reason for hiding this comment

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

[suggestion] Now that there are multiple components in this file related to expired subscriptions, e.g. see the following below around line ~100:

{subscriptions.subscriptionPlan && subscriptions.showExpirationNotifications && <SubscriptionExpirationModal />}

... it might be a bit confusing to have 2 different components with similar responsibilities (i.e., ExpiredSubscriptionModal and SubscriptionExpirationModal), especially when rendered in different places within DashboardPage.

A couple considerations:

  1. If we do keep both as separate modals, I might recommend rendering the related modal components together, co-located, within DashboardPage, ideally with explicit/clear documentation (e.g., comments, perhaps an ADR) why there's multiple related components pertaining to subscription expiration.
  2. Consider extending/re-purposing the existing SubscriptionExpirationModal to include the custom modal messaging use case. The component is currently responsible for showing the default "expiring" or "expired" subscriptions modal, rendered only on the Dashboard page today. Perhaps it could also be responsible for showing the custom subscription expiration messaging, too?
    • For example, for a learner with an expired subscription license, when custom subscription expiration messaging is configured, show the custom expiration modal or otherwise fallback to the default expiration modal if no custom messaging is configured. That said, if we go this route, I might recommend creating sub-components within SubscriptionExpirationModal to avoid making the single component that exists today larger / difficult to parse (e.g., CustomSubscriptionExpiredModal, SubscriptionExpiredModal, SubscriptionExpiringModal).
    • However, while easier to reason about, this approach would likely mean showing the existing subscription expiration messaging on the search/course routes where it is not present today. That said, adding the existing expiration modal messaging to these additional routes is likely actually desirable.
    • Implementation-wise, the conditional rendering of SubscriptionExpirationModal that exists today (i.e., subscriptions.subscriptionPlan && subscriptions.showExpirationNotifications) would also need to move inside SubscriptionExpirationModal in order to support the custom expiration modal use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamstankiewicz I agree with your suggestion!
We can create a follow-up ticket to repurpose the existing SubscriptionExpirationModal to handle the custom subscription expiration messaging. For now, we can render the related modal components together, with clear comments explaining the purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adamstankiewicz A ticket has been created for this purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the ticket link!

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM with 1 requested change before it merges to remove the named enterprise customer in the comments.

src/components/dashboard/DashboardPage.jsx Outdated Show resolved Hide resolved
@mahamakifdar19 mahamakifdar19 merged commit 6aaebbb into master Oct 21, 2024
6 checks passed
@mahamakifdar19 mahamakifdar19 deleted the maham/ENT-9617 branch October 21, 2024 07:52
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