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

break up setRoute into separate methods? #11

Open
albertalquisola opened this issue Feb 6, 2017 · 2 comments
Open

break up setRoute into separate methods? #11

albertalquisola opened this issue Feb 6, 2017 · 2 comments

Comments

@albertalquisola
Copy link

albertalquisola commented Feb 6, 2017

This is more of a usability enhancement vs. new feature but what do you think about breaking up the setRoute function into separate methods? I dislike how 'overloaded' it is. Depending on the params you pass in, the function will perform slightly different actions, making it hard to reason about and use.

RIght now, setRoute can be used in 4 different ways:

  1. setRoute(route)
  • navigate to new route
  1. setRoute(start, length)
  • remove a segment from the current route
  1. setRoute(index, value)
  • replace a segment of the current route
  1. setRoute(start, length, value)
  • remove and set a segment on the current route

Caveat is that this will introduce breaking changes to the router (I guess we could make it backwards compatible but not a huge fan of that idea)

Also, I'm willing to submit a PR for this if you think it's a good idea

Thoughts?

@albertalquisola
Copy link
Author

went ahead and wrote some of the new potential APIs to get a better sense of how theyd look:

Router.prototype.removeSegment = function(options) {
  var url = this.explode();

  if (start < url.length) {
    url.splice(options.start, options.length);
  }

  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.replaceSegment = function(options) {
  var url = this.explode();
  url[options.index] = options.value;

  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.removeAndSetSegment = function(options) {
  var url = this.explode();

  if (options.start < url.length) {
    url.splice(options.start, options.length, options.value);
  }

  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.setRoute = function(options) {
  var url = [options.route].join('/');
  listener.setHash(url.join('/'), options.state);

  return url;
};

Router.prototype.replaceRoute = function(options) {
  var url = [options.route];
  listener.replaceHash(url.join('/'), options.state);

  return url;
};

@mhulse
Copy link

mhulse commented Feb 7, 2017

Sounds like a good idea, and looks much more readable to me. Why don't you do a PR, with updated tests, so others can do functional reviews? Thanks @albertalquisola!!!!

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

No branches or pull requests

2 participants