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

Add active state to Popover2 when popover is open and target is a button #1872

Merged
merged 9 commits into from Dec 9, 2017

Conversation

gjavitt
Copy link
Contributor

@gjavitt gjavitt commented Dec 7, 2017

Checklist

Changes proposed in this pull request:

Adds active state to Popover2 when popover is open and target is a button. (Does not make button active if popover is triggered by hover interaction instead of click.)

Screenshot

image

const target: JSX.Element = React.cloneElement(children.target, {
// force disable single Tooltip child when popover is open (BLUEPRINT-552)
className: classNames({ [Classes.ACTIVE]: isOpen && !isHoverInteractionKind }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to make sure we don't overwrite custom target classes that might have been provided. Let's move this to the classNames composition on L282:

targetProps.className = classNames(
    Classes.POPOVER_TARGET,
    {
        [Classes.ACTIVE]: isOpen && !isHoverInteractionKind,
        [Classes.POPOVER_OPEN]: isOpen,
    },
    className,
);

Copy link
Contributor

Choose a reason for hiding this comment

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

@cmslewis targetProps.className or target.className?

Copy link
Contributor

Choose a reason for hiding this comment

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

targetProps. Here's the code that's already present:

image

cmslewis
cmslewis previously approved these changes Dec 7, 2017
Copy link
Contributor

@cmslewis cmslewis left a comment

Choose a reason for hiding this comment

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

Whoop there it is. 👍

@cmslewis cmslewis dismissed their stale review December 7, 2017 01:29

Got trigger happy. pt-active is actually added to a wrapper around button children. Needs more investigation.

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

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

.pt-active should be on target, not targetProps. Requesting changes so @cmslewis and I can think about a better implementation

@adidahiya
Copy link
Contributor

@gjavitt please merge master into your branch to pick up the new CI workflows

@llorca
Copy link
Contributor

llorca commented Dec 8, 2017

@giladgray is this PR cool? if not, can you think of a better approach?

const target: JSX.Element = React.cloneElement(children.target, {
className: classNames({ [Classes.ACTIVE]: isOpen && !isHoverInteractionKind }),
Copy link
Contributor

Choose a reason for hiding this comment

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

must add children.target.props.className to avoid clobbering existing classes.

@giladgray
Copy link
Contributor

@gjavitt can you please enable preview comments for your Circle build so we can play with it?

@gjavitt
Copy link
Contributor Author

gjavitt commented Dec 8, 2017

unclobber classnames

Preview: documentation | landing | table

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

this code change looks good.

worth noting that it does not work for the nested <Popover><Tooltip><Button /></Tooltip></Popover> use case because the class does not get applied to the Button itself (see Tooltip example in docs). don't think there's much we can do about this.

@llorca
Copy link
Contributor

llorca commented Dec 8, 2017

Agree Gilad, if anything it's something we should fix on the Tooltip/popover-target side, it's biting us in many places

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

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

nice work @gjavitt!

@cmslewis cmslewis merged commit adddb94 into palantir:master Dec 9, 2017
cmslewis pushed a commit that referenced this pull request Dec 13, 2017
…ton (#1872)

* Add active state to Popover2 when popover is open and target is a button

* Add active state to Popover2 when popover is open and target is a button

* fix

* back to target instead of targetProps

* fix

* back to target instead of targetProps

* unclobber classnames
cmslewis added a commit that referenced this pull request Dec 13, 2017
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.

None yet

5 participants