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

Fix missing cursor for ternary plot #7057

Merged
merged 9 commits into from
Jul 22, 2024
Merged

Conversation

Lexachoc
Copy link
Contributor

Fix #7056

This is my first contribution to such a large open source project. If I understand the coding style wrong or wrong logic for the fix, please let me know.

draftlogs/7057_fix.md Outdated Show resolved Hide resolved
@archmoj archmoj added bug something broken community community contribution status: reviewable labels Jul 22, 2024
Lexachoc and others added 3 commits July 22, 2024 19:11
Co-authored-by: Mojtaba Samimi <[email protected]>
update as suggusted
@Lexachoc
Copy link
Contributor Author

I also updated the src/plots/ternary/index.js:

-    if(dragmode === 'pan') {
-        toplevel.style('cursor', 'move');
-    } else {
-        toplevel.style('cursor', 'crosshair');
-    }
+ toplevel.style('cursor', dragmode === 'pan' ? 'move' : 'crosshair');

Comment on lines 102 to 104
var toplevel = this.plotContainer.selectAll('g.toplevel');

toplevel.style('cursor', dragmode === 'pan' ? 'move' : 'crosshair');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use _ternarylayer.

Suggested change
var toplevel = this.plotContainer.selectAll('g.toplevel');
toplevel.style('cursor', dragmode === 'pan' ? 'move' : 'crosshair');
var fullLayout = gd._fullLayout;
fullLayout._ternarylayer
.selectAll('g.toplevel')
.style('cursor', fullLayout.dragmode === 'pan' ? 'move' : 'crosshair');

Copy link
Contributor Author

@Lexachoc Lexachoc Jul 22, 2024

Choose a reason for hiding this comment

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

I have updated again with your sugguestion.
Also, I moved the updateFx function from proto.plot to the function Ternary(options, fullLayout) function so that the updateFx function is only triggered once at initialization instead of being called every time the plot is zoomed/panned and resized. What do you think?

update and clean as suggested
update and clean as suggested

exports.updateFx = function(gd) {
var fullLayout = gd._fullLayout;

Copy link
Contributor

Choose a reason for hiding this comment

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

Please run npm run lint -- --fix and commit the changes.

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I hope I'm doing it correctly.

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
💃

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.

Cursor missing for ternary plot
2 participants