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: migrate setting modals #54112
feat: migrate setting modals #54112
Conversation
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.
Did a quick test locally and the modals LGTM 👍
I'll do another round of testing once #54139 is in.
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.
Merged the latest main and the changes work great.
Though I noticed that when the modal is open, the close button is focused, but there is no focus outline around it. This issue cannot be reproduce in ui-components
/ Storybook, so I think it has something to do with /learn and its CSS overrides.
I'll create a ticket to track the issue separately.
Updated: Scratch this, I mixed up the interaction during testing. The modal is behaving as expected:
- If the user opens the modal via mouse click, the close button is automatically focused but there is no focus outline (it's expected that the focus outline doesn't show up)
- If the user opens the modal via keyboard, the close button is focused and there is a focus outline
Also, the Modal component currently aligns its body content to the left. I have created #54161 to allow customizing modal body alignment. We mostly want the body content to center, but there are some cases where the content should be left-aligned.
Thank you for the hard work. I had to review this in a couple of rounds. Here are the suggestions for the first round: client/src/components/SolutionViewer/project-modal.tsx client/src/components/settings/delete-modal.tsx client/src/components/settings/reset-modal.tsx client/src/components/signout-modal/index.tsx client/src/templates/Challenges/components/reset-modal.tsx |
@ahmaxed Thank you for the review, and I agree on the header styles needing to be fixed (it's one of those cases where /learn override the ui-components styles).
This one I was consider removing, tbh 😄 But I think it's better to keep unrelated changes in a separate PR. Though could you elaborate on this change:
I assume you are referring to the "Close" button in the footer. This "Close" button is currently positioned on the right in prod (we didn't make any changes to this). Are you suggesting moving all of them to the left as a UI improvement, or did we make an unintended change somewhere and need to revert it? |
It would be wonderful if you could prioritize the modal header styles after this pr. |
I've updated the PR with the following changes:
|
Checklist:
main
branch of freeCodeCamp.client/src/components/profile/components/time-line.tsx
client/src/components/settings/delete-modal.tsx
client/src/components/settings/reset-modal.tsx
client/src/components/signout-modal/index.tsx
client/src/components/SolutionViewer/exam-results-modal.tsx
client/src/components/SolutionViewer/project-modal.tsx
client/src/components/staging-warning-modal/index.tsx
Tests
client/src/components/profile/components/time-line.tsx
(Tested solution modal onsolution-viewer.spec.ts
)client/src/components/settings/delete-modal.tsx
(Tested ondelete-modal.spec.ts
)client/src/components/settings/reset-modal.tsx
(Tested onreset-modal.spec.ts
)client/src/components/signout-modal/index.tsx
(Tested onsignout-modal.spec.ts
)client/src/components/SolutionViewer/exam-results-modal.tsx
(Tested onexam-reslts.spec.ts
)client/src/components/SolutionViewer/project-modal.tsx
(Tested solution modal onsolution-viewer.spect.ts
)client/src/components/staging-warning-modal/index.tsx
(Tested onstaging-warning-modal.spec.ts
)Ref #52759