Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enhancement: setting delay to show/hide popover #267

Merged
merged 9 commits into from
Feb 4, 2019
Merged

Conversation

feliupe
Copy link

@feliupe feliupe commented Jan 27, 2019

Is this a bug fix or enhancement?

Enhancement.

Is there a related issue?

I want to make possible to set an delay to show/hide a popover. Just like the Bootstrap Tooltip.

It can be very annoying being unable to set in cases like tables, where you have all the cells around also wrapped in a popover. If you hover really fast from on side to the other of the table you will be unable to see some cells as the popover is shown the moment you hover one on it side.

Let me know if you don't get it. I can add screenshots later.

Any Breaking changes?

Yes. Thought, I can work around it, by not removing transitionDuration for now.


Please let me know if it sounds like something for you, so that I can work on it better - with your help - and also write tests.

We use all over the client where I work, and we would be pretty much interested in having it merged.

@coveralls
Copy link

coveralls commented Jan 27, 2019

Coverage Status

Coverage decreased (-0.1%) to 97.328% when pulling 7578092 on feliupe:master into a95fdb2 on wxsms:master.

@feliupe feliupe changed the title doing changes and changing documentation Enhancement: setting delay to show/hide popover Jan 30, 2019
@wxsms
Copy link
Member

wxsms commented Jan 31, 2019

Hi, thank you for using this lib. the idea sounds good, but:

  1. transitionDuration has nothing to do with delays, this props is to indicate how fast the hiding / showing transition is, therefore this should not be a breaking change, and should not have any affect to this prop. you should use a new prop, something like delay, which contents {show: Number, hide: Number}
  2. pls fix lint errors after making changes
  3. it's better to have unit tests for this feature

Copy link
Member

@wxsms wxsms left a comment

Choose a reason for hiding this comment

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

pls see my comment

@feliupe
Copy link
Author

feliupe commented Jan 31, 2019

Hi @wxsms , thanks for your reply.

  1. I am sorry, but maybe I am missing or misunderstanding something.

this props is to indicate how fast the hiding / showing transition is[...]

I can't see any use of transitionDuration for showing anything related to popUpMixin.js - and the only two components that are apparently using it are the Tooltip and the Popover.

There is only one reference, when hiding the popup:

https://github.com/wxsms/uiv/blob/a95fdb2f9c5d7fc925dabc8d85a5915fd3a86ae2/src/mixins/popupMixin.js#L214-L225

One more thing that I've noticed is that it is not quite a duration for hidding it.

Inside the setTimeout callback it's only being removed from the DOM. The hide itself, as I see, happens outside, and before with removeClass(this.$refs.popup, SHOW_CLASS). As I see, this serves solely to wait a bit before the css fade transition of hiding it happens. If you set it to 20000 it will be hidden in less than one second anyways.

As a sidenote I would be also happy in documenting (writing an example) better this transition-duration, if you could help with it by explaining the desired result to me.

  1. done
  2. done

Thanks :)

Copy link
Member

@wxsms wxsms left a comment

Choose a reason for hiding this comment

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

pls see my comment in code.

this.hideTimeoutId = setTimeout(() => {
removeClass(this.$refs.popup, SHOW_CLASS)
// gives fade out time
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

There might be a bug: while you clearing hideTimeoutId, if the inner code have already run, the inner timeout will not be cleared. and it will still be removed from DOM while time arrived.

Copy link
Author

Choose a reason for hiding this comment

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

hi @wxsms , you are right. Please check out my last commit 7578092

Thanks :)

@wxsms wxsms changed the base branch from master to dev February 4, 2019 15:53
@wxsms wxsms merged commit 23b6e16 into uiv-lib:dev Feb 4, 2019
@wxsms
Copy link
Member

wxsms commented Feb 4, 2019

@feliupe done. will release on next version. thanks.

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

Successfully merging this pull request may close these issues.

3 participants