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

Fix setPlaceholder to update this.options.placeholder #502

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

marexandre
Copy link
Contributor

@marexandre marexandre commented Jul 5, 2023

Fix setPlaceholder so that when setPlaceholder is called afterwards it return updated placeholder string

Before

Example of the bug and what this PR tries to fix:

const geocoder = new MapboxGeocoder({
    accessToken: mapboxgl.accessToken,
    mapboxgl: mapboxgl,
    placeholder: 'Test'
});

geocoder.getPlaceholder();
// returns Test"

geocoder.setPlaceholder('New string');

geocoder.getPlaceholder();
// returns  "Test"  and not the updated placeholder text

After

const geocoder = new MapboxGeocoder({
    accessToken: mapboxgl.accessToken,
    mapboxgl: mapboxgl,
    placeholder: 'Test'
});

geocoder.getPlaceholder();
// returns  "Test"

geocoder.setPlaceholder('New string');

geocoder.getPlaceholder();
// returns  "New string" 
  • 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

@marexandre
Copy link
Contributor Author

If I wanted to have this change in v4 of this library, what would be the recommendations to add a similar fix?

@marexandre
Copy link
Contributor Author

CI is failing for the following reason - which does not seem to be related to changes in this PR...

Screen Shot 2023-07-05 at 12 03 23 PM

Copy link
Contributor

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

Hi @marexandre,

Thanks for the contribution, overall everything looks good to me 👍
Unfortunately, CI will fail for external contributors because the access token isn't set there. But it should pass locally when you add the MapboxAccessToken environment variable.

@marexandre
Copy link
Contributor Author

@stepankuzmin thank you for taking a look at this changes - rant the tests locally and all passed 🎉
How should we proceed with merging this PR? Anything else I should do or missed?

@stepankuzmin stepankuzmin merged commit bf1c5a1 into mapbox:main Jul 18, 2023
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