search

sparksuite / react-accessible-dropdown-menu-hook

A simple Hook for creating fully accessible dropdown menus in React
http://sparksuite.github.io/react-accessible-dropdown-menu-hook
MIT License
112 stars 26 forks source link

Fix for demo website crashing when keyboard controls are used #261

Closed corymharper closed 3 years ago

corymharper commented 3 years ago

This pull request fixes a bug in the demo website that caused it to crash any time keyboard controls were used on the dropdown menu.

Closes #260

codecov[bot] commented 3 years ago

Codecov Report

Merging #261 (4e99307) into master (f280137) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #261   +/-   ##
=======================================
  Coverage   91.86%   91.86%           
=======================================
  Files           1        1           
  Lines          86       86           
  Branches       24       24           
=======================================
  Hits           79       79           
  Misses          7        7

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f280137...4e99307. Read the comment docs.

WesCossick commented 3 years ago

Can you explain the changes in this PR? You mentioned some things in Slack but we need to document the change here.

corymharper commented 3 years ago

Can you explain the changes in this PR? You mentioned some things in Slack but we need to document the change here.

Sure, I'll explain my whole process... I started out thinking that it was something to do with the linked package, I remarked as much in the issue while I was exploring what was going on, because the website worked properly after unlinking the package and using the published version, and updating the lockfile.

However, it seemed like that couldn't be the case, because one of our test projects also links to the package internally. So, converting back to the linked version, I then ran yarn install in the demo website's directory, because the lockfile needed updating after changing dependencies. The website still worked properly whereas at base it was displaying the same error we experience in the version published to Github Pages. So, it seems like some versions of our packages were probably out of alignment and causing unexpected errors. Unfortunately, I am not sure which specific package was the source of the error currently, or if it was multiples of them.

The one other change in this pull request, the addition of two type packages, was a requirement after updating the dependencies of the website project, otherwise tsc would fail while saying some node_module was using an import typed as any because it was lacking types.

My method for testing if upgrading the packages really fixed the error was to start at base, so basically the master branch, run yarn clean and yarn dev, and start up the demo website, confirming that the expected error was propagating. I would then terminate the process, reintroduce the changes, and repeat the above process of yarn clean/yarn dev > start the demo website. Then confirming that the error was no longer propagating.

This probably displays a need for puppeteer driven tests very similar to what is done in the testing website but for the demo, as that would have caught this bug I'm sure, however I was thinking that might be the subject of a separate pull request.

WesCossick commented 3 years ago

Yeah, maybe in the future we can run tests against the demo website itself to make sure it's functioning properly. It's less critical, though, than the actual code that gets published with the package.