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

Expand and scroll container when opening menu #3669

Closed

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Jul 6, 2019

Fixes #3473
Fixes #1267

This PR does three main things:

  1. Fixes the above-mentioned issues by correctly identifying and using the height of a scroll parent when the select component is inside one. This means that the menu will no longer be only partially visible and force the user to scroll the parent in order to see the rest.

  2. Allows the menu to be sized a bit smaller in cases where it can fit inside the scroll parent without causing the scroll parent to grow. This is preferable to expanding the scroll parent and causing the select component to jump to a new position, caused by the open menu.

  3. Updates cypress to the latest version (I was having a hard time getting the existing version to run correctly in newer versions of chrome), and adds a new e2e test file for menus.

IanVS added 4 commits July 6, 2019 11:22
If the parent scroll container is not tall enough to contain the menu,
constrain the height of the menu so that it fits inside the container.
The older version of cypress does not work correctly in watch mode
with newer versions of chrome.
There weren’t any tests specifically around the menu, so this adds a
second group of tests specifically for them, as well as a new script
command for `e2e-watch`.
@IanVS
Copy link
Contributor Author

IanVS commented Sep 26, 2019

Here's the message from CI for the e2e failure. Any help in fixing this would be appreciated!

yarn run v1.9.4
$ concurrently --kill-others --success=first --names 'SERVER,E2E' 'yarn start --progress=false --no-info' 'yarn test:cypress'
[SERVER] $ cd docs && yarn start --progress=false --no-info
[E2E] $ cypress run --spec ./cypress/integration/select_spec.js
[E2E] The cypress npm package is installed, but the Cypress binary is missing.
[E2E] 
[E2E] We expected the binary to be installed here: /root/.cache/Cypress/3.3.2/Cypress/Cypress
[E2E] 
[E2E] Reasons it may be missing:
[E2E] 
[E2E] - You're caching 'node_modules' but are not caching this path: /root/.cache/Cypress
[E2E] - You ran 'npm install' at an earlier build step but did not persist: /root/.cache/Cypress
[E2E] 
[E2E] Properly caching the binary will fix this error and avoid downloading and unzipping Cypress.
[E2E] 
[E2E] Alternatively, you can run 'cypress install' to download the binary again.
[E2E] 
[E2E] https://on.cypress.io/not-installed-ci-error
[E2E] 
[E2E] ----------
[E2E] 
[E2E] Platform: linux (Debian - 8.11)
[E2E] Cypress Version: 3.3.2
[E2E] error Command failed with exit code 1.
[E2E] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
[E2E] yarn test:cypress exited with code 1

@IanVS
Copy link
Contributor Author

IanVS commented Nov 22, 2019

If anyone wants to try this out, you can use this commit from our fork, which includes the generated files: nutshellcrm@c231943

@bladey bladey added pr/needs-review PRs that need to be reviewed to determine outcome pr/bug-fix PRs that are specifically addressing a bug and removed uncertain labels Jun 4, 2020
@ebonow ebonow added the menu-bug Addresses menu positioning, scrolling, or general interactions label Jan 16, 2021
@IanVS
Copy link
Contributor Author

IanVS commented Apr 26, 2021

If there's a chance this could get merged in, I can spend some time rebasing and getting it ready. Let me know if y'all will have time to review at some point.

@ebonow
Copy link
Collaborator

ebonow commented Apr 26, 2021

@IanVS Thanks for circling back on this. We'll be exploring some long overdue menu positioning behavior fixes following the release of the TypeScript refactor. Either way, it's frustrating that the PR review process is so far behind, but hoping we can make more headway once we've got v5 settled.

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Nov 11, 2021

@IanVS Hey, we're finally circling back to this. We apologize it took so long to get to this after the work you've put into it.

Right now we're planning to use Popper for menu positioning. The changes you've made to the positioning logic are great, however there are a few things I would like to tweak about it. Given that we're switching to Popper anyway, would you be willing to scale back this PR or create a new PR to just include the new Cypress tests (they can be skiped if they fail) so that we can use those for testing with Popper? They provide a really good starting point for creating a menu positing test suite and we want to give you credit for putting in the work.

@Methuselah96
Copy link
Collaborator

@IanVS Also, if you don't have time to work on this at the moment, no worries. I've started #4913 which is inspired by the Cypress tests added in this PR. I can incorporate your tests into that PR and close out this PR if that's easier, but if you want credit for it I'd be happy to review/merge this PR with just the tests when it's ready.

@IanVS
Copy link
Contributor Author

IanVS commented Nov 13, 2021

Hi, thanks for the consideration, but it if you're able to take what I started here and run with it, I'm more than happy with that. I'm stretched a bit thin, and not using react-select at the moment. I'm glad that the PR was useful, even if only as a guide or starting point. Feel free to copy whatever's useful and close this PR. Best of luck with everything!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
menu-bug Addresses menu positioning, scrolling, or general interactions pr/bug-fix PRs that are specifically addressing a bug pr/needs-review PRs that need to be reviewed to determine outcome
Projects
None yet
4 participants