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

Fretboard OnClick, clickable area slighty off from note position #27

Open
Aduffy opened this issue Jan 6, 2021 · 5 comments
Open

Fretboard OnClick, clickable area slighty off from note position #27

Aduffy opened this issue Jan 6, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@Aduffy
Copy link

Aduffy commented Jan 6, 2021

If you click on the bottom part of a note on String 6, it returns that note from string 1
View console for output.
https://codepen.io/aduffy/pen/dypvjKa?editors=0001

@moonwave99 moonwave99 added the bug Something isn't working label Jan 6, 2021
@moonwave99
Copy link
Owner

The hover div does not resize accordingly, that's the problem ^^

Screenshot 2021-01-07 at 14 08 30

Will look into that, thanks for reporting!

@Aduffy
Copy link
Author

Aduffy commented Feb 2, 2021

Hey Diago I know your a busy guy right now, but any chance you might have a chance to look at this bug?

@moonwave99
Copy link
Owner

I have it a look under xmas and looks trickier than expected (due to svg scaling / padding).

Atm I have definitely 0% time unfortunately. Hope to find some in the next weeks!

PhilJollans added a commit to PhilJollans/fretboard.js that referenced this issue Sep 29, 2024
I have made two changes:
 - pass topPadding into the function and subtract it from the Y position
 - compare the Y coordinate to 0.5, 1.5, ... times the string separation

Previously, the Y coordinate was compared to 1, 2, ... times the separation.
I think this only worked, when topPadding was approximately half of the string separation.
@PhilJollans
Copy link

PhilJollans commented Sep 29, 2024

I have created a pull request (#67) which I think will fix this problem.

The strings group is offset vertically from hoverDiv by topPadding, so this should be subtracted from the Y coordinate.

At the same time, the Y coordinate is compared to 1 * stringDistance for the first string, up to 6 * stringDistance for the sixth string. This is not really correct. We ought to compare the Y coordinate to 0.5 * stringDistance up 6.5 * stringDistance.

This previously worked more or less correctly if topPadding was approximately half the string separation, in which case the two effect cancelled each other out. If you change the height option, making the strings closer or further apart, it is easy to generate an error.


Sorry, I might have been a bit too quick with my pull request. There still appears to be a problem, in particular if you set the height to a low value, like 75 or even 50.

@PhilJollans
Copy link

I have closed my first pull request, but now made a second pull request (#68) with a new fix to the problem.

The size of the hoverDiv was unreliable, in particular if you started resizing the window. This could change the size of the SVG diagram, but it did not modify the bottom margin. I have simply dumped the bottom margin, setting it to zero.

This however means the hoverDiv is typically too big and can generate events away from the fretboard (in the fret numbers). To counter that I have changed the return type of getPositionFromMouseCoords to return undefined if the mouse position is more than half the string separation outside the fretboard.

Determining the position was actually more complicated than necessary. In particular, it now only fetches the strings group, and calculates the position relative to its top left corner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants