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

added tabletmode to fire plotly_click event for mobile devices. #6563

Merged
merged 4 commits into from
Apr 21, 2023

Conversation

NickTominaga
Copy link
Contributor

I added tabletmode flag and touchstart event to fire plotly_click event on scatter3d plot for mobile devices.

@archmoj archmoj added community community contribution status: reviewable feature something new labels Apr 13, 2023
if(selection.buttons && selection.distance < 5) {
if(selection.buttons && selection.distance < 5 && !tabletmode) {
gd.emit('plotly_click', eventData);
} else if(tabletmode && selection.distance < 5) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR.
Could we possibly combine the two if statements for plotly_click like this?

        if(selection.distance < 5 && (selection.buttons || tabletmode)) {
            gd.emit('plotly_click', eventData);
        } else {
            gd.emit('plotly_hover', eventData);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review I just updated the code and added draft log but probably I misunderstood the usage of the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like my suggestion caused the tests to fail.
Let's revert that part but please keep the draftlog.
And we should ready to go 🚀
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I just took your review which only changed draftlogs but the test passed. The same thing happened between 82604eb and 735f8f2

In that commit I just eslinted but the test passed too. webgl-jasmine fails sometimes. weird...

@archmoj
Copy link
Contributor

archmoj commented Apr 19, 2023

I think we could call this a bug fix.
So please add a draft log i.e. draftlogs/6563_fix.md as explained here.

@archmoj archmoj added bug something broken and removed feature something new labels Apr 19, 2023
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
@archmoj
Copy link
Contributor

archmoj commented Apr 21, 2023

💃

@archmoj archmoj merged commit 45a4d98 into plotly:master Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken community community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants