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

feat: migrate setting modals #54112

Merged
merged 27 commits into from Mar 29, 2024

Conversation

Sembauke
Copy link
Member

@Sembauke Sembauke commented Mar 15, 2024

Checklist:

  • 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 on solution-viewer.spec.ts )
  • client/src/components/settings/delete-modal.tsx (Tested on delete-modal.spec.ts)
  • client/src/components/settings/reset-modal.tsx (Tested on reset-modal.spec.ts)
  • client/src/components/signout-modal/index.tsx (Tested on signout-modal.spec.ts)
  • client/src/components/SolutionViewer/exam-results-modal.tsx (Tested on exam-reslts.spec.ts)
  • client/src/components/SolutionViewer/project-modal.tsx (Tested solution modal on solution-viewer.spect.ts)
  • client/src/components/staging-warning-modal/index.tsx (Tested on staging-warning-modal.spec.ts)

Ref #52759

@github-actions github-actions bot added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. labels Mar 15, 2024
@Sembauke Sembauke marked this pull request as ready for review March 19, 2024 07:15
@Sembauke Sembauke requested a review from a team as a code owner March 19, 2024 07:15
Copy link
Member

@huyenltnguyen huyenltnguyen left a 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.

client/src/components/settings/delete-modal.tsx Outdated Show resolved Hide resolved
client/src/components/solution-display-widget/index.tsx Outdated Show resolved Hide resolved
client/src/components/staging-warning-modal/index.tsx Outdated Show resolved Hide resolved
client/src/templates/Challenges/components/reset-modal.css Outdated Show resolved Hide resolved
e2e/delete-modal.spec.ts Outdated Show resolved Hide resolved
e2e/solution-viewer.spec.ts Outdated Show resolved Hide resolved
e2e/solution-display-widget.spec.ts Show resolved Hide resolved
@huyenltnguyen huyenltnguyen self-assigned this Mar 21, 2024
Copy link
Member

@huyenltnguyen huyenltnguyen left a 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.

@huyenltnguyen huyenltnguyen added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Mar 22, 2024
@ahmaxed
Copy link
Member

ahmaxed commented Mar 27, 2024

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
Close button should be on the right.

client/src/components/settings/delete-modal.tsx
Head text color, and Text weight
Close button should be on the right. (why do we have three close buttons. :) )

client/src/components/settings/reset-modal.tsx
Head text color, and Text weight

client/src/components/signout-modal/index.tsx
Head text color, and Text weight

client/src/templates/Challenges/components/reset-modal.tsx
Head text color, and Text weight

@huyenltnguyen
Copy link
Member

@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).

Close button should be on the left. (why do we have three close buttons. :) )

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:

Close button should be on the left.

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?

@huyenltnguyen huyenltnguyen added status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels Mar 27, 2024
@ahmaxed
Copy link
Member

ahmaxed commented Mar 27, 2024

It would be wonderful if you could prioritize the modal header styles after this pr.
the close button on the footer is on the right in some instances and on the left in some.

@huyenltnguyen
Copy link
Member

huyenltnguyen commented Mar 28, 2024

I've updated the PR with the following changes:

  • Overriding h2 styles that /learn sets in order to have the modal title displayed correctly
    • I don't like polluting the global.css file. But either that or we have to update Modal.Header to accept className and having a CSS file for each modal component in /learn
  • Adding the new alignment prop to Modal.Footer to support the alignment of the Close button
    • I did not add a Storybook demo for this prop as we will need to do some cleanup first. It's because we currently have a single alignment value used for Modal.Body, but Modal.Footer now also needs one. I'll raise a separate PR to sort out the Storybook setup.
  • Correcting the alignment of the Close button in the project-modal footer
  • Removing the Close button from delete-modal, as well as moving the other two buttons from Modal.Body to Modal.Footer for consistency

Screenshots of the modals after the changes
Modal Screenshot
project-modal Screenshot 2024-03-28 at 11 58 32
delete-modal Screenshot 2024-03-28 at 11 58 14

@huyenltnguyen huyenltnguyen added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Mar 28, 2024
@ahmaxed ahmaxed enabled auto-merge (squash) March 29, 2024 12:30
@ahmaxed ahmaxed merged commit 76b4c81 into freeCodeCamp:main Mar 29, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants