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

Update active node sizing based on zoom #20

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

wgolledge
Copy link
Contributor

Hi and thank you for the great extension! Like some others, I have started using it with Foam.

Summary

I noticed that while the node size is updated based on zoom, the active node is not; this was bugging me when I zoomed in and the active node hid part of the node label.

This PR makes it so that the active node size is also determined via d3 instead of css (based on ACTIVE_RADIUS) and changes zoomActions to be a resize func that is invoked on every file change.

Please do let me know what you think and if there's any changes you'd like.

Before - node size while zoomed-in

zoom bug

After - active node size is updated along with the other nodes

size updates based on scroll

@wgolledge wgolledge force-pushed the bugfix/zoom-active-circle branch from 4768c77 to ffb10d3 Compare July 5, 2020 15:04
Comment on lines 176 to 178
svg.selectAll('circle').filter(function() {
return d3.select(this).attr("active");
}).attr("r", zoomOrKeep(ACTIVE_RADIUS));
Copy link
Owner

Choose a reason for hiding this comment

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

I know this is not covered in CI yet, but please stick to Prettier formatting rules which gives:

svg
  .selectAll('circle')
  .filter(function() {
    return d3.select(this).attr('active');
  })
  .attr('r', zoomOrKeep(ACTIVE_RADIUS));

BTW, can it be done in a way to avoid using function and this? IIRC this callback is given a param (usually called d) which contains some information. Maybe active can be extracted from it too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tchayen thanks a lot for taking a look!

I have updated it to use a function accessing the DOM element via the callback params like you mentioned.

I think that .attr(...) is probably still the best way to access the active attribute.

@tchayen
Copy link
Owner

tchayen commented Jul 7, 2020

Thanks for the PR! I have obviously missed the update of active node. One small change request from me and then I think I am ok with merging it.

@tchayen
Copy link
Owner

tchayen commented Jul 8, 2020

Great! Thanks a lot! Merging it now. I am planning to make a bugfix release soon, but I am quite busy lately so I can only say that it will eventually happen but don't know when.

@tchayen tchayen merged commit 78baab7 into tchayen:master Jul 8, 2020
@wgolledge wgolledge deleted the bugfix/zoom-active-circle branch July 8, 2020 15:20
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.

2 participants