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: use component library's dropdown component in learn #50465

Merged
merged 21 commits into from Jun 22, 2023

Conversation

ahmaxed
Copy link
Member

@ahmaxed ahmaxed commented May 22, 2023

This pr includes

  • feat: make ui-components share same presets as learn
  • feat: improve dropdown styles
  • feat: add hot reloading for component library CSS
  • feat: add component library to learn

Remaning tasks:
1 - extract types declarations from the component library and import them to learn.
2 - style the toggle button when toggled to look like the hover state using headless ui's internal state.
3 - include the linking scripts in package.json.
4 - build and publish the component library upon approval.

Checklist:

Closes #XXXXX

@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 May 22, 2023
@codesee-maps
Copy link
Contributor

codesee-maps bot commented May 22, 2023

👀 Review this PR in a CodeSee Review Map

View the CodeSee Map of this change

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

feat: improve dropdown styles

feat: add hot reloading for component library CSS

feat: add componet library to learn
@ojeytonwilliams
Copy link
Contributor

Hey @ahmadabdolsaheb what's the plan re: publishing? Are we going to publish to npm first or rework this to use the local code?

@ahmaxed
Copy link
Member Author

ahmaxed commented May 22, 2023

We could do either of the followings:
1-Split this into two prs (learn and ui-components). Once the ui-components pr is merged, I could publish and then we could merge the learn one.
2-publish the component library from the pr and then merge this pr.
Either way I am happy.

@ojeytonwilliams
Copy link
Contributor

Both seem reasonable to me. Option 2 sounds like it might waste less time, so do you want to go with that?

@ahmaxed
Copy link
Member Author

ahmaxed commented May 24, 2023

Package published: https://www.npmjs.com/package/@freecodecamp/ui

@ahmaxed
Copy link
Member Author

ahmaxed commented May 24, 2023

Another round of reviews then merge?

@ojeytonwilliams
Copy link
Contributor

@ahmadabdolsaheb it looks like there are a couple of issues.

Firstly pnpm defaults to using the @freecodecamp/ui workspace. This is probably fine, but means we need to build it before linting etc.

Secondly, the types aren't quite working correctly. To fix this we'll need a "types" property in the package.json of ui-components, and point it at the types. Also we're not generating the declaration files any more. I could have sworn we did at one point!

@socket-security
Copy link

socket-security bot commented Jun 1, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 2, 2023

The timeline jest tests need a little work. After that, it should be good to go.
The dropdown menu items don't show up in the jest render for some reason.

@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 8, 2023

Jest to e2e test migration could be addressed in an upcoming pr to unblock this pr.
Two tests need to be migrated to e2e:

  1. certification settings.
  2. timeline.

Either a multifile project needs to be added to the certified seed object or cypress needs to submit one so it shows up as the last one on the timeline.

@ahmaxed ahmaxed marked this pull request as ready for review June 8, 2023 14:14
@ahmaxed ahmaxed requested a review from a team as a code owner June 8, 2023 14:14
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Hey @ahmadabdolsaheb the new dropdown looks great, but doesn't seem as friendly from an a11y perspective.

Right now, in production, it's possible to tab onto a dropdown, press enter to expand and then use the arrow keys to navigate between the options.

In this PR all of that is works except for the arrow key navigation, so it's not possible to make a selection.

I'd rather we delayed a bit than shipped a less accessible version.

client/tailwind.config.js Outdated Show resolved Hide resolved
@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 9, 2023

@ojeytonwilliams should be fixed.

@bbsmooth
Copy link
Contributor

bbsmooth commented Jun 9, 2023

Thank you @ojeytonwilliams, the pnpm upgrade did the trick. I'll try to remember that next time.

I'll put this through its paces and write up any notes soon.

@bbsmooth
Copy link
Contributor

bbsmooth commented Jun 9, 2023

Here are my notes for the Get Help menu in the JS ADS challenges.

Tested with: http://localhost:8000/learn/javascript-algorithms-and-data-structures/basic-javascript/compound-assignment-with-augmented-subtraction50465

Should be fixed:

  • A nice improvement is that now when the Get Help button is activated with keyboard, focus is placed on the first item in the menu. Unfortunately, this breaks when using either the JAWS or NVDA screen reader. I will figure out why this is happening and let you know. [More info below. This appears to be a bug in the headlessui menu implementation so nothing we can do about it ourselves.]

  • Get Help -> Get a Hint: We should add sr-only text "opens in new window" to the end of the menu text here (this will be an improvement over current version on production). Should also consider adding the opens in new window icon that we use in the main menu.

  • Get Help -> Watch a Video: When closing the video either by Esc key or 'x' close button, focus should be placed back on Get Help button (this is current behavior on production)

  • Get Help -> Ask for Help: When closing the help dialog either by Esc key, 'x' close button, or Cancel button, focus should be placed back on Get Help button (this is current behavior on production)

Nice to have but not required:

  • Allow the up/down arrow keys to wrap around in the menu (this is current behavior on production)

  • In addition to the background color changing on focus of menu item, add the blue outline (using :focus-visible) as well for consistency (this is current behavior on production)

@ojeytonwilliams
Copy link
Contributor

Thanks for the meticulous review, @bbsmooth. One thing I'm curious about is this: can we automate this kind of QA? At least now that you've pointed out what needs to be happening.

Some unit tests with react-testing-library's tools would be good, assuming they can catch the issues you described.

@bbsmooth
Copy link
Contributor

bbsmooth commented Jun 9, 2023

More information on the issue with keyboard focus not automatically being placed on the first menu item when using JAWS/NVDA. This menu implementation is using aria-activedescendant to handle the focus management. Using the keyboard without a screen reader works properly. When the Get Help button is activated, the menu div is inserted into the DOM and the aria-activedescendant attribute (on the menu div) is set to the id of the first item in the menu. This is also (surprisingly) working properly when using the Orca screen reader in Linux.

But when using JAWS/NVDA, the aria-activedescendant attribute is NOT being set on the menu div when activating the Get Help button with the keyboard. I'm noticing that if you click on the Get Help button with the mouse that the aria-activedescendant attribute is also not set on the menu div either. So I'm assuming the Get Help button is attempting to determine whether the user used a keyboard to click the button or a pointer device, and only wants to set aria-activedescendant when using a keyboard. Unfortunately, whatever method the button is using to make this distinction isn't working when using JAWS/NVDA.

I'm also finding this bug on the headless ui menu example. So this seems like a bug with the component itself and not something we are doing. I guess I'll need to put together a codepen demo and open an issue in their github.

@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 10, 2023

Thanks for the meticulous review, @bbsmooth. Just have a few questions regarding the dropdown menu implementation:

Allow the up/down arrow keys to wrap around in the menu (this is current behavior on production)

Does this mean to have the icon wrap to the next line on smaller screens?

Get Help -> Get a Hint: We should add sr-only text "opens in new window" to the end of the menu text here (this will be an improvement over current version on production). Should also consider adding the opens in new window icon that we use in the main menu.

I have no idea how to start implementing this, so pointers are appreciated.

@bbsmooth
Copy link
Contributor

Allow the up/down arrow keys to wrap around in the menu (this is current behavior on production)

Does this mean to have the icon wrap to the next line on smaller screens?

Sorry, I might not have been as clear as I could have been. If focus is currently on the top item in the menu, pressing the up arrow key moves focus to the bottom item. Likewise, if focus is currently on the bottom item, pressing the down arrow key moves focus to the top item.

This is not required keyboard functionality, just nice to have, and the menu on production currently has it.

@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 12, 2023

It seems like bootstrap modal finds the last active element before the modal is open and set .focus() to that element when the modal is closed.
Resources: modal, baseModal, activeElements

@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 13, 2023

@bbsmooth, this should cover our bases. I was wondering if we could move forward without the followings since they need to be resolved on headlessui's side?

  • Allow the up/down arrow keys to wrap around in the menu (this is current behavior on production)
  • A nice improvement is that now when the Get Help button is activated with keyboard, focus is placed on the first item in the menu. Unfortunately, this breaks when using either the JAWS or NVDA screen reader. I will figure out why this is happening and let you know. [More info below. This appears to be a bug in the headlessui menu implementation so nothing we can do about it ourselves.]

@Sembauke Sembauke requested a review from bbsmooth June 14, 2023 11:47
@bbsmooth
Copy link
Contributor

I was wondering if we could move forward without the followings since they need to be resolved on headlessui's side?

Yes, definitely. I don't think either of these issues are blockers. The arrow wrap is definitely not (just nice to have). The lack of focus on the first menu item when using NVDA/JAWS is slightly more annoying but won't keep these users from using the menu. I plan on creating an issue for this when I have a few minutes to put together a good demo.

@bbsmooth
Copy link
Contributor

Well, I found another "issue" but I don't think there is anything we can do about it (this seems to be a bug in headlessui). When you open the menu with a keyboard and it places focus on the first menu item, if you then hit the Tab key it closes the menu and places the focus on the next focusable item on the page after the "Get Help" button (which is the JS editor in this case). That is the expected behavior. But if you do the same thing but use Shift + Tab then it still places you in the JS editor instead of going backwards and placing the focus on the "Reset this lesson" button, which is the previous focusable element before the "Get Help" button. I guess this is another issue to open with headlessui. Shouldn't stop us from going forward.

And just one very minor nitpick under our control. If you don't think it is worthy of fixing now, that's fine. If it really bugs me I can open my own PR.

focus-outline-cut-off

As you can see in the above pic, when zoomed in, the left side of the focus outline is cut off so only 1px is showing. I think setting outline-offset: -3px on these menu items for focus-visible/hover will solve that problem. We have this same issue for the Help/Reset/Run buttons at the bottom, but that's perhaps a little out of scope for this PR so I will open a new PR for those (unless you want to deal with them now).

Otherwise, the other fixes look good and this is ready to go.

Co-authored-by: Naomi Carrigan <nhcarrigan@gmail.com>
@ahmaxed
Copy link
Member Author

ahmaxed commented Jun 21, 2023

@bbsmooth, the changes that are in our control from your latest comment are in.

Copy link
Contributor

@bbsmooth bbsmooth left a comment

Choose a reason for hiding this comment

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

LGTM.

@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 Jun 22, 2023
@moT01 moT01 merged commit eb22bc0 into freeCodeCamp:main Jun 22, 2023
18 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

6 participants