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

Node modulize, return active group this on activation #10

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mhkeller
Copy link

No description provided.

@mhkeller
Copy link
Author

needs to add var d3 = require('d3') most likely within the module.exports if statement. will check if that's the best spot for it.

@mhkeller
Copy link
Author

Okay this should be good both in the browser and browserify environments. I've added two new option methods, which are also in the readme and index file but you can change the language / organization if you like. They are

  • .offset, which takes a number and offsets the activation of the next section by that many pixels. Was previously hardcoded to 180.
  • .triggerAt, this defaults to 'top' if not set, which is the existing behavior: the next section becomes active when it reaches the top of the viewport. It can also be 'middle', which will trigger the next section when it reaches the middle of the viewport. Similar offset rules apply in both instances.

@1wheel
Copy link
Owner

1wheel commented Feb 20, 2016

awesome. quick assorted thoughts/notes of things to look into more:

  • not sure about the direction as the second arg; think previous index might be more generally useful (can do something different if skipping forward or back) and nicer to use (i < prev v. `direction === 'down').
  • might be misunderstanding, but is triggerAt needed? couldn't you use offset(innerHeight/2)? was trying to avoid hitting any slow browser APIs in the reposition loop (innerHeight might not be slow though?).
  • should this arg be set to the html node to be consistent w/ the rest of d3? or is that silly since it'll be used at d3.select(this) 95% of the time.
  • does this require boilerplate work with rollup/how do I make this a d3 plugin?

@mhkeller
Copy link
Author

  • Ah ya I didn't make the connection between what the second arg could be. Adding an example in the docs should be enough for people to figure out direction
  • I looked for some performance info on measuring the viewport but couldn't find much. We were talking about having the user put graphscroll.offset() inside their resize function and give an example in the doc. Thinking about it more, putting it in user land like that makes more sense since you could disable it in mobile view where your layout is probably very different.
  • The advantage for this from my usage is if I want to grab a data- attribute, I can do this.dataset.myvalue and not have to bring d3 into it. Measuring the section width could also be useful with .getBoundingClientRect()
  • Not sure how to package it as a d3 plugin but the current changes shouldn't break the existing using – just allow it to be used in a node environment

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