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 challenge modals #54179

Merged
merged 30 commits into from Apr 3, 2024

Conversation

Sembauke
Copy link
Member

@Sembauke Sembauke commented Mar 22, 2024

Checklist:

ref #52759

@github-actions github-actions bot added the platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. label Mar 22, 2024
@github-actions github-actions bot added the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Mar 22, 2024
@huyenltnguyen
Copy link
Member

The jsx-a11y/label-has-associated-control issue that CodeFactor flag is a false positive.

The label text of the checkboxes are not plain text / static, but require some computation. The ESLint rule couldn't find any plain text so it thinks the label is empty / invalid. There is an issue similar to this: jsx-eslint/eslint-plugin-jsx-a11y#966.

I'm confident that our implementation is correct because:

  • I tested with a screen reader, and the label of the checkboxes are announced properly.
  • We query the checkboxes using getByRole in Playwright tests, with the name of the checkboxes being the label text, the the tests are able to find those checkboxes in the DOM.
    const rsaCheckbox = page.getByRole('checkbox', {
    name: 'I have tried the Read-Search-Ask method'
    });
    const similarQuestionsCheckbox = page.getByRole('checkbox', {
    name: 'I have searched for similar questions that have already been answered on the forum'
    });

@github-actions github-actions bot added the scope: i18n language translation/internationalization. Often combined with language type label label Mar 27, 2024
@Sembauke Sembauke marked this pull request as ready for review March 29, 2024 08:39
@Sembauke Sembauke requested a review from a team as a code owner March 29, 2024 08:39
@ahmaxed ahmaxed added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Mar 29, 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.

@Sembauke I made some changes to the PR as I tested the changes locally.

The changes are:

  • Handling the onKeyDown event in completion-modal
    • This also includes adding the onKeyDown prop to the Modal component)
  • Restoring the !canFocusEditor condition in hotkeys.tsx (f3adb37#diff-0f97911cdf2b391a4029ce7d981cdd922bd45c140c8a7afd55282c79bc04f959R204)
    • I think there is a bug in the focus management that prevents us from being able to trigger the shortcuts modal consistently. It's probably related to the last point in Most common incorrect answers for step-1 #54024 (comment), so I think we should look into the issue separately (and add more tests to cover the expected behaviors)
  • Adding closeButtonClassnames='close' to Modal.Header so that the close button is displayed properly
    • There are some CSS rules in /learn that targets the button element, which overrides ui-components styles. And thus, we need to pass the close class name from /learn, which is tied with some CSS styles, to compete the CSS specificity
  • Adding xLarge Modal size to accommodate for the video-modal
  • Updating the modal padding value to accommodate for small screen sizes

@ahmaxed ahmaxed merged commit a39e740 into freeCodeCamp:main Apr 3, 2024
22 checks passed
@Sembauke Sembauke deleted the feat/migrate-challenge-modals branch April 3, 2024 12:28
ahmaxed pushed a commit to ahmaxed/freeCodeCamp that referenced this pull request Apr 4, 2024
Co-authored-by: Huyen Nguyen <25715018+huyenltnguyen@users.noreply.github.com>
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: i18n language translation/internationalization. Often combined with language type label 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