Skip to content

Conversation

@jacbn
Copy link
Contributor

@jacbn jacbn commented Oct 31, 2025

(Please see #1660 for prior context)

This takes on many of the suggestions implemented by the PR above, but does away with the separation between stateful and non-stateful modals. Since we now rely exclusively on the ID in Redux to store which modal(s) are open, we are free to fill the contents of the modals with state if they so require, though in truth I do not believe they should ever need any given they exist as sources of information, not interactivity.

I have moved the pagination logic to inside ActiveModal, as I believe this is going to become a very common use case given various discussions. To allow this, what is passed to body now determines whether pagination occurs; if this is a list of items, each item is interpreted as its own page in the modal, and a "Next" button is used to go from one to the next (TODO: "Previous"?). This also means the same logic / animation will be used to travel between the pages, when this gets built.

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

❌ Patch coverage is 73.17073% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.64%. Comparing base (99a861b) to head (5670abd).
⚠️ Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
src/test/pages/TeacherOnboardingModal.cy.tsx 0.00% 10 Missing and 1 partial ⚠️
.../elements/modals/TeacherEventConfirmationModal.tsx 0.00% 7 Missing ⚠️
src/app/components/elements/modals/ActiveModal.tsx 91.17% 2 Missing and 1 partial ⚠️
...p/components/elements/modals/ReservationsModal.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1809      +/-   ##
==========================================
+ Coverage   41.56%   41.64%   +0.08%     
==========================================
  Files         543      543              
  Lines       23784    23844      +60     
  Branches     7863     7929      +66     
==========================================
+ Hits         9885     9931      +46     
- Misses      13256    13267      +11     
- Partials      643      646       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jacbn jacbn marked this pull request as ready for review November 5, 2025 10:21
Copy link
Contributor

@axlewin axlewin left a comment

Choose a reason for hiding this comment

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

It's nice to have some tidier logic here, and the teacher onboarding modal looks great with the animations.

The only issue I've found is with non-paginated modals: it seems to be counting each character in the body as its own page, which yields some interesting modals:

Image

Otherwise this all looks good 👍

@jacbn
Copy link
Contributor Author

jacbn commented Nov 7, 2025

Ah, strings are structurally very similar to arrays so a modal with a body: "some string" was passing the Symbol.iterator in Object(activeModal.body) check. I could change it to a simple Array.isArray() check, but there are other types of iterator that it would make sense to accept. Looking at the list (and excluding those that are not valid ReactNodes that would not type anyway), strings are the only edge case so I have just added a typeof body !== "string" check.

@axlewin axlewin merged commit 74f2490 into main Nov 7, 2025
10 checks passed
@axlewin axlewin deleted the active-modal-refactor branch November 7, 2025 15:14
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.

6 participants