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

There was an addtional div when LogoControl was removed #4813

Closed
jingsam opened this issue Jun 9, 2017 · 3 comments · Fixed by #4842
Closed

There was an addtional div when LogoControl was removed #4813

jingsam opened this issue Jun 9, 2017 · 3 comments · Fixed by #4842

Comments

@jingsam
Copy link
Contributor

jingsam commented Jun 9, 2017

mapbox-gl-js version:
0.37.0

Steps to Trigger Behavior

https://jsfiddle.net/jingsam/ue99mkgy/1/

Expected Behavior

there should not exsit a mapbox-ctrl div

Actual Behavior

_20170609182456

@jfirebaugh
Copy link
Contributor

Is this causing a particular issue? The logo control is always preset by default; whether it shows anything is dependent on what style you load and your Mapbox account.

@jingsam
Copy link
Contributor Author

jingsam commented Jun 10, 2017

Noop, but I would expect the logo control to be hidden when there were no mapbox sources.

After I checked the logo_control.js, I found the implemention was rather buggy. For example:
https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/control/logo_control.js#L26
this._updateLogo() would do nothing because the parameter e is undefined

https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/control/logo_control.js#L47
I think it should be this._map.off('sourcedata', this._updateLogo); Another question is why close the sourcedata event? If users remove all mapbox sources and then readd mapbox sources, this._updateLogo may not fire again.

The logic of _updateLogo is also problematic. When this._logoRequired() is false, the logoControl should be hidden but not to be removed.

I think a simpler and cleaner logic to implement the logoControl is: creating all necessary div and a tag in onAdd, and then if there exists any mapbox sources, display the control; if not, hide this control with display: none

@mollymerp
Copy link
Contributor

@jingsam thanks for the flag. You're right, there are some issues w the logic in LogoControl. I'm working on a PR that I think will address your issues.

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 a pull request may close this issue.

3 participants