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.

Sign up for GitHub

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

Jump to bottom

inline=X ⇒ usePortal=!X #2045

Merged
merged 13 commits into from
Feb 1, 2018
Merged

inline=X ⇒ usePortal=!X #2045

merged 13 commits into from
Feb 1, 2018

Conversation

giladgray
Copy link
Contributor

Fixes #1341

Changes proposed in this pull request:

  • inline=X ⇒ usePortal=!X
    • Overlay/Dialog default usePortal=true
    • 🔥 Popover/Tooltip default usePortal=false (cuz Popper.js is really great)
  • rewrite docs about this prop

Gilad Gray added 3 commits January 26, 2018 15:09
inline=X => usePortal=!X
c4844f4 
Overlay/Dialog default usePortal=true
:fire: Popover/Tooltip default usePortal=false (cuz Popper.js is really great)
uses of the _other_ inline prop
93da1d3
overlay and popover docs
46008a4
@giladgray giladgray requested a review from adidahiya January 26, 2018 23:20
@giladgray giladgray changed the title inline=X => usePortal=!X inline=X ⇒ usePortal=!X Jan 26, 2018
@giladgray giladgray changed the title inline=X ⇒ usePortal=!X inline=X ⇒ usePortal=!X Jan 26, 2018
@blueprint-bot
Copy link

overlay and popover docs

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix menu test

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix select test

Preview: documentation | landing | table

@adidahiya
Copy link
Contributor

Popover/Tooltip default usePortal=false (cuz Popper.js is really great)

please elaborate on this.

@giladgray
Copy link
Contributor Author

I have found that in simple cases inline Popovers (powered by Popper.js) work identically to portaled popovers. This was not the case with Tether, but "Popper.js is really great," so things just work now.

I also found that in most cases I prefer to use inline Popovers cuz they're easier to style and reason about. So with the improved functionality from Popper.js, seems reasonable to change the default.

@adidahiya
Copy link
Contributor

How does it work? Is Popper creating its own portal-like thing under the hood?

@giladgray
Copy link
Contributor Author

no, Popper is just better at positioning than Tether.
usePortal is really only necessary if you have overflow: hidden on an ancestor but you want the popover to be able to overflow that ancestor. classic example is the table row use case.

@blueprint-bot
Copy link

Merge branch 'develop' into gg/usePortal

Preview: documentation | landing | table

@blueprint-bot
Copy link

oops

Preview: documentation | landing | table

adidahiya
adidahiya suggested changes Jan 29, 2018
View reviewed changes
Copy link
Contributor

@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.

if Popper isn't pulling its contents out of the DOM tree and attaching it in another spot, then I'm strongly against the default change. it requires users to think about z-stacking and overflow more than necessary.

@giladgray
Copy link
Contributor Author

giladgray commented Jan 29, 2018 •
edited
Loading

@adidahiya play with this example for a bit, look at the inline styles that Popper injects in each case, and then get back to me: https://9326-71939872-gh.circle-artifacts.com/0/home/circleci/project/packages/docs-app/dist/index.html#core/components/popover.portal-rendering (updated link)

@blueprint-bot
Copy link

revert Popover default value: usePortal=true now

Preview: documentation | landing | table

adidahiya
adidahiya approved these changes Jan 30, 2018
View reviewed changes
packages/docs-app/src/examples/core-examples/collapsibleListExample.tsx
@@ -72,6 +72,7 @@ export class CollapsibleListExample extends BaseExample<ICollapsibleListExampleS
<RadioGroup
key="collapseFrom"
name="collapseFrom"
inline={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the other inline modifier, the one that makes checkboxes etc display: inline-block so they'll lie on the same line

@blueprint-bot
Copy link

Merge branch 'develop' of github.com:palantir/blueprint into gg/usePortal

Preview: documentation | landing | table

@giladgray
Copy link
Contributor Author

#2053 will fix the test failures here because it removes those tests (MenuItem dynamic layout)!

@blueprint-bot
Copy link

refactor Overlay background scrolling tests

Preview: documentation | landing | table

@blueprint-bot
Copy link

fix popover examples usePortal

Preview: documentation | landing | table

@giladgray giladgray merged commit 0845be5 into develop Feb 1, 2018
@giladgray giladgray deleted the gg/usePortal branch February 1, 2018 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adidahiya adidahiya adidahiya approved these changes

Assignees
No one assigned
Labels
Type: breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@giladgray @blueprint-bot @adidahiya