-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
feat(Popup): add closeOnClickInside support #2037
feat(Popup): add closeOnClickInside support #2037
Conversation
e918581
to
7a4247a
Compare
</Popup> | ||
) | ||
assertInBody('.ui.popup.visible') | ||
domEvent.click(document.getElementById('click-me')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ in manual tests, clicking this seems to work per expectation
7a4247a
to
2d12e09
Compare
Codecov Report
@@ Coverage Diff @@
## master #2037 +/- ##
==========================================
+ Coverage 99.76% 99.76% +<.01%
==========================================
Files 148 148
Lines 2561 2562 +1
==========================================
+ Hits 2555 2556 +1
Misses 6 6
Continue to review full report at Codecov.
|
2d12e09
to
e4c20b6
Compare
woo hoo. test & linting fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdaringe Thanks for PR, I added some comments, but all of them about styling 👍
src/addons/Portal/Portal.js
Outdated
|
||
if (!closeOnPortalClick) return | ||
|
||
debug('handlePortalClick()') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of our methods have debug()
call first. I think that this can refactored to this:
handlePortalClick = (e) => {
debug('handlePortalClick()')
const { closeOnPortalClick } = this.props
if (closeOnPortalClick) this.close(e)
}
src/modules/Popup/Popup.js
Outdated
@@ -247,7 +250,7 @@ export default class Popup extends Component { | |||
getPortalProps() { | |||
const portalProps = {} | |||
|
|||
const { on, hoverable } = this.props | |||
const { on, hoverable, closeOnClickInside } = this.props |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's sort variables by name
src/modules/Popup/Popup.js
Outdated
@@ -270,6 +273,9 @@ export default class Popup extends Component { | |||
portalProps.mouseLeaveDelay = 70 | |||
portalProps.mouseEnterDelay = 50 | |||
} | |||
if (closeOnClickInside) { | |||
portalProps.closeOnPortalClick = true | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be one-line:
if (closeOnClickInside) portalProps.closeOnPortalClick = true
formatting changes completed and pushed |
Hey guys, quick question on naming here. After giving this a few days to sit and coming back to it, the If a component supports a click event, it is usually just <Portal closeOnClick />
<Popup closeOnClick /> For the Portal on click seems to obviously apply to the portal. For the Popup it seems a click "inside" is a given. Am I over thinking this? |
@layershifter / @cdaringe any thoughts on the above? If I don't hear back, I may go ahead with the rename at the next release (after today's release). |
Sorry for the delay. I ended up just using the open plus onClick props.
I'm wondering if it's even necessary. It is nice for quick prototyping.
Sorry for the headache! What do you think?
…On Sep 17, 2017 6:18 PM, "Levi Thomason" ***@***.***> wrote:
@layershifter <https://github.com/layershifter> / @cdaringe
<https://github.com/cdaringe> any thoughts on the above? If I don't hear
back, I may go ahead with the rename at the next release (after today's
release).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2037 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA9O_VJ38-WyX_7eWZ8m6AlVByZs4e4Iks5sjcT-gaJpZM4PJtSE>
.
|
In my mind, the fewer options the better :) Will close for now. Thanks for the work anyway. |
Fixes #2034.
problem statement
onclick
solution