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

Add enable option #52

Merged
merged 1 commit into from
Aug 19, 2022
Merged

Add enable option #52

merged 1 commit into from
Aug 19, 2022

Conversation

tremby
Copy link
Contributor

@tremby tremby commented Aug 18, 2022

An enable option is added as a new third argument, which defaults to true. It can be set to false to disable geolocation. The idea is that the option can be flipped to true once the user has indicated that they want to be geolocated.

This means the hook can be made conditional without violating the rules of hooks and without a conditional nested component which would need to pass its data back up the tree.

Closes #51


Before this changeset, the first argument is a PositionOptions object and the second is a callback.

I considered a few ways to add this new enable option:

  • Should it be a third argument? (non-breaking)

  • Or extend PositionOptions? (probably non-breaking)

  • Or make it the first argument and bump the existing ones to 2 and 3 (breaking)

  • Or perhaps change the structure completely to an options object, such as

    interface UseGeolocationOptions {
      enable?: boolean;
      positionOptions?: PositionOptions;
      callback?: (geolocation: EnrichedGeolocationCoordinates) => void;
    }
    
    function useGeolocation(options?: UseGeolocationOptions) ...

    (breaking, unless we add extra code to check for legacy options style)

I went with the first option since it is simplest and non-breaking.

You might consider changing to the scheme in the 4th option or similar in a future major release -- it'd be tidier and more developer-friendly IMO.


After I wrote my test, I found that another test (specifically "useGeolocation when geolocation data is not available handles initial error") broke. This is because a particular mock was left with an implementation after my test which intentionally was not firing, and this mock was not reset before that other test was run, and that other test expected it not to be mocked. So I removed the specific resets, and added a blanket reset all mocks after each test across the whole suite.

An `enable` option is added as a new third argument, which defaults to
`true`. It can be set to `false` to disable geolocation. The idea is
that the option can be flipped to `true` once the user has indicated
that they want to be geolocated.

This means the hook can be made conditional without violating the rules
of hooks and without a conditional nested component which would need to
pass its data back up the tree.

Closes bence-toth#51
@tremby tremby force-pushed the add-enable-option branch from 6d66a70 to 767d0c7 Compare August 18, 2022 23:23
@bence-toth bence-toth self-assigned this Aug 19, 2022
@bence-toth bence-toth merged commit b4cb10f into bence-toth:main Aug 19, 2022
@bence-toth
Copy link
Owner

Hi @tremby, thank you for this kind contribution.

This has been now released with v1.1.0.

@tremby tremby deleted the add-enable-option branch August 19, 2022 21:32
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.

Add an option to disable geolocation
2 participants