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

[popover2] feat: Popover2 and Tooltip2 components #4488

Merged
merged 61 commits into from
Jan 21, 2021
Merged

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Jan 20, 2021

Fixes #4023

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Creates a new package @blueprintjs/popover2
    • This package depends on popper.js v2 and react 16.8+
    • This package contains ports of Popover and Tooltip to use the new version of the positioning library, with minimal changes to the API
  • Notable changes compared to <Popover>:
    • There is no more .bp3-popover-wrapper wrapper element surrounding the target and overlay. Both the target and overlay are rendered out to the virtual DOM as siblings.
    • There is an option to no longer render a .bp3-popover-target wrapper element surrounding the target as well. If renderTarget prop is supplied, its return value will be rendered out directly to the DOM without a wrapper.
      • Note: in this case it is the consumer's responsibility to disable a <Tooltip2> target which is a direct child of a <Popover2> when the popover is open (<Popover> would handle this automatically for you, with its wrapper element behavior).
    • The old API of supplying a target as the first child of <Popover2> is retained. In this case, a .bp3-popover-target wrapper element will be generated around the target child, so we can atttach event handlers to it.
    • For certain prop configurations (openOnTargetFocus={true} and hover interaction kinds), a tabIndex was previously generated and applied to the cloned props.children element directly. Now, that tabIndex is applied to the bp3-popover2-target wrapper element to increase the chance of this feature working in more situations.

Reviewers should focus on:

  • DX of the new renderTarget API

Screenshot

(pretty much identical to Popover)

image

Not addressed in this PR

@blueprint-bot
Copy link

core: fix lint

Previews: documentation | landing | table

Copy link
Contributor Author

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

self-review

packages/core/test/popover/popoverTests.tsx Outdated Show resolved Hide resolved
packages/core/test/popover/popoverTests.tsx Show resolved Hide resolved
packages/docs-app/package.json Outdated Show resolved Hide resolved
packages/popover2/src/tooltip2.tsx Outdated Show resolved Hide resolved
packages/popover2/test/popover2Tests.tsx Outdated Show resolved Hide resolved
packages/popover2/test/popover2Tests.tsx Outdated Show resolved Hide resolved
packages/popover2/test/testUtils.ts Outdated Show resolved Hide resolved
packages/popover2/test/tooltip2Tests.tsx Outdated Show resolved Hide resolved
@blueprint-bot
Copy link

fix lint

Previews: documentation | landing | table

@blueprint-bot
Copy link

fix more lint

Previews: documentation | landing | table

@adidahiya adidahiya merged commit 2d4d188 into develop Jan 21, 2021
@adidahiya adidahiya deleted the ad/popover2 branch January 21, 2021 02:37
@trixn86
Copy link

trixn86 commented Jan 26, 2021

Not addressed in this PR

  • eventually we should move to jest + puppeteer for unit testing

Have you considered using cypress.io for end-to-end/integration tests? In my (personal) opinion this library has many benefits over jest + selenium. The test runner itself is a free open source library and I haven't had so much fun writing end-to-end test with another tool yet. It's very fast and intuitive. They also provide a dashboard service that has a free plan for open source projects.

@adidahiya
Copy link
Contributor Author

@trixn86 I've looked at cypress but not used it myself yet. We need fast unit tests in Blueprint, so I don't think we can fully switch over to an ETE test suite and retain the kind of developer experience we currently have in this repo. I imagine that spinning up all the services necessary for ETE tests is slower than mocha+karma or jest+puppeteer, but if I'm wrong about that do let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Popper.js v2.0
3 participants