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

Get & set options #226

Merged
merged 8 commits into from
Mar 22, 2019
Merged

Get & set options #226

merged 8 commits into from
Mar 22, 2019

Conversation

scottsfarley93
Copy link

@scottsfarley93 scottsfarley93 commented Mar 20, 2019

This PR fixes #176 by adding getter/setter methods.

This PRs get/set methods for the following options:

  • language
  • zoom
  • flyTo
  • placeholder
  • bbox
  • countries
  • types
  • minLength
  • limit
  • filter
  • proximity (already implemented)

It's an open question as to whether we should support get, and particularly set, methods for these options, since changing their behavior more fundamentally alters the behavior of the plugin -- and I'm not sure of the use case of changing them after creating an instance with different values:

  • accessToken
  • origin
  • localGeocoder
  • reverseMode
  • reverseGeocode

#208 will add get/set functions for html rendering functions and we should add analogous methods in #219 to support new options that we're adding in version 4 (#197)

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • run npm run docs and commit changes to API.md
  • update CHANGELOG.md with changes under master heading before merging

\cc @katydecorah @yuletide

@scottsfarley93 scottsfarley93 changed the base branch from master to version4 March 20, 2019 21:53
@andrewharvey andrewharvey added this to the v4.0.0 milestone Mar 21, 2019
@scottsfarley93 scottsfarley93 changed the title [WIP] Get & set options Get & set options Mar 21, 2019
Copy link
Contributor

@katydecorah katydecorah left a comment

Choose a reason for hiding this comment

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

This brings sooo much customization 😍

I hear you on wanting to be consistent on these get and set options, but I think it's also fair to not add options for the few that you listed your PR comment. It's hard to think of valuable use cases for them. Nevertheless, not that we have these patterns in place it should be fairly straightforward to add if someone requests a getter/setter that we don't already have.

I left you several comments with some (nitpicky) documentation suggestions, please take or leave as you see fit.

lib/index.js Outdated

/**
* Set the zoom level
* @param {Number} zoom On geocoded result what zoom level should the map animate to when a `bbox` isn't found in the response. If a `bbox` is found the map will fit to the `bbox`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Small copy suggestion:

Suggested change
* @param {Number} zoom On geocoded result what zoom level should the map animate to when a `bbox` isn't found in the response. If a `bbox` is found the map will fit to the `bbox`.
* @param {Number} zoom The zoom level that the map should animate to when a `bbox` isn't found in the response. If a `bbox` is found the map will fit to the `bbox`.

lib/index.js Outdated
},

/**
* Sets the flyTo options
Copy link
Contributor

Choose a reason for hiding this comment

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

Small edit for consistency:

Suggested change
* Sets the flyTo options
* Set the flyTo options

lib/index.js Outdated
},

/**
* Gets the bounding box used by the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency:

Suggested change
* Gets the bounding box used by the plugin
* Get the bounding box used by the plugin

lib/index.js Outdated
},

/**
* Sets the bounding box to limit search results to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Sets the bounding box to limit search results to
* Set the bounding box to limit search results to

lib/index.js Outdated
},

/**
* Set the countries to limit search results to
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be types?

Suggested change
* Set the countries to limit search results to
* Set the types to limit search results to

lib/index.js Outdated
},

/**
* Get the limit value used by the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Get the limit value used by the plugin
* Get the limit value for the number of results to display used by the plugin

lib/index.js Outdated
},

/**
* Get the minimum length used in the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call out what the minimum length refers to?

Suggested change
* Get the minimum length used in the plugin
* Get the minimum number of characters typed to trigger results used in the plugin

lib/index.js Outdated
},

/**
* Set the minimum length value used by the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Set the minimum length value used by the plugin
* Set the minimum number of characters typed to trigger results used by the plugin

lib/index.js Outdated
},

/**
* Set the limit value used by the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Set the limit value used by the plugin
* Set the limit value for the number of results to display used by the plugin

},

/**
* Get the filter function used by the plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not exactly sure what filter does in suggestions, maybe we could add a little more to add context?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@scottsfarley93 scottsfarley93 merged commit ee926a2 into version4 Mar 22, 2019
@scottsfarley93 scottsfarley93 deleted the get-set-options branch March 22, 2019 22: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.

3 participants