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

Fixed click counter persisting #4630

Merged
merged 2 commits into from
Dec 31, 2017
Merged

Fixed click counter persisting #4630

merged 2 commits into from
Dec 31, 2017

Conversation

ajlomagno
Copy link
Contributor

Fixes #4605

The code that controls the left click and right click portions of the welcome walkthrough removes the click counter element upon successfully clicking the required 5 times then moving onto the next chapter of the walkthrough. It does not take into account the possibility of the user clicking onto a later portion of the walkthrough while the clicking portions still have not been completed, skipping the deletion of the element altogether.

This fix adds a line to curtain.js that deletes any leftover counter elements when constructing the new tooltip.

id-fix

@bhousel
Copy link
Member

bhousel commented Dec 27, 2017

This fix adds a line to curtain.js that deletes any leftover counter elements when constructing the new tooltip.

Great! It looks ok - would it work if the removal code is moved to here instead?:

chapter.exit = function() {
listener.off();
};

Each chapter has an exit method that is called when the user leaves that chapter, so I think it makes more sense for cleanup code to be there. It's not really the curtain's job to clean up stray DOM elements.

@ajlomagno
Copy link
Contributor Author

That sounds like a much better way to fix this. I originally was looking for a way to contain the cleanup code within that chapter but I seemed to have missed the exit method.

I'll make a new commit very soon and we can take another look at it. Thanks!

@ajlomagno
Copy link
Contributor Author

@bhousel I fixed up this pr with what you mentioned earlier on. I believe it's good to go now.

@bhousel
Copy link
Member

bhousel commented Dec 31, 2017

Thanks! yay first-time contributor!

@bhousel bhousel merged commit 573158e into openstreetmap:master Dec 31, 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.

Click counter stays around if you interrupt that walkthrough step
2 participants