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

Don't fire sourcedata event with sourceDataType: 'metadata' when a source is removed #5771

Closed
wants to merge 5 commits into from

Conversation

jfirebaugh
Copy link
Contributor

Noticed this while working on #5762 -- this was added in #4842, but it seems odd to me to fire this event when the source itself isn't changing. Instead, have the LogoControl listen for the styledata event that's triggered when a source is removed.

…urce is removed

This event doesn't really make sense for this scenario; the metadata isn't changing. Instead, have the LogoControl listen for the styledata event that's triggered when a source is removed.
Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

This change works for updating the logo control, but it changes the behavior of the attribution control when a source is removed. I think you'll have to add a listener for styledata on the AttributionControl as well to make sure removed sources don't continue to appear there.

@@ -32,6 +32,7 @@ class LogoControl {
this._container.appendChild(anchor);
this._container.style.display = 'none';

this._map.on('styledata', this._updateLogo);
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll also need to remove this event listener in the onRemove function

@@ -32,6 +32,7 @@ class LogoControl {
this._container.appendChild(anchor);
this._container.style.display = 'none';

this._map.on('styledata', this._updateLogo);
this._map.on('sourcedata', this._updateLogo);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are safe to remove this listener all together. Keeping it here and removing the metadata condition causes _updateLogo to be called on every tile load 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might still be necessary to ensure that the control updates when TileJSON is loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, thank you! it was working on the debug page because the spritesheet was loading after the tilejson.

@jfirebaugh jfirebaugh self-assigned this Feb 27, 2018
@jfirebaugh
Copy link
Contributor Author

Stale; closing for now.

@jfirebaugh jfirebaugh closed this Jun 28, 2018
@jfirebaugh jfirebaugh deleted the styledata-events branch June 28, 2018 18:00
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