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

conditional postclick execution onNext #4062

Merged
merged 2 commits into from May 15, 2017

Conversation

Projects
None yet
5 participants
@bagnacan
Copy link
Contributor

commented May 12, 2017

If you run the Galaxy UI tour, you will obtain a strange behavior while clicking the "Prev" button right before triggering the "File Upload".
The desired behavior would be:

  • the current bubble disappears
  • the previous bubble appears

what happens instead is that

  • the current bubble disappears
  • the file upload dialog (the next step) opens nonetheless, and is shown in the background
  • the previous bubble appears

The problem is that the postclick events are executed onHide, regardless of which step the user intended to land on. I propose a little solution that executes the postclick if onNext. Might be just a partial solution, but it solves the problem

@nsoranzo

This comment has been minimized.

Copy link
Member

commented May 12, 2017

@galaxybot test this

@nsoranzo nsoranzo requested a review from dannon May 12, 2017

@@ -32,10 +32,15 @@ define(['libs/bootstrap-tour'],function(BootstrapTour) {
}
if (step.postclick){
step.onHide = function(){
_.each(step.postclick, function(postclick){

This comment has been minimized.

Copy link
@dannon

dannon May 12, 2017

Member

This is a good idea. That said, it looks to me like step.onNext should completely replace the parent step.onHide block, otherwise it looks like what would happen here is that onHide, we'd actually be attaching new onNext functions every time, right?

This comment has been minimized.

Copy link
@bagnacan

bagnacan May 12, 2017

Author Contributor

I agree on the replacement of the onHide with the onNext.
I was a bit hesitant on replacing it in the first place, since I thought we might want to have an onHide behavior that might differ in case an onPrev or an onNext was triggered, but I now think this is not the case. What do you think?

This comment has been minimized.

Copy link
@dannon

dannon May 12, 2017

Member

Yep, I agree, probably makes the most sense to replace it completely for now. If we do have need to have a non-onNext triggered event, we can always add it in at that point.

@galaxybot galaxybot added this to the 17.09 milestone May 12, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@bagnacan Thanks for the modification, looks perfect to me!

@dannon dannon merged commit 3a9df37 into galaxyproject:dev May 15, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.