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

compact mapbox wordmark on narrow maps #6472

Merged
merged 6 commits into from
May 15, 2018
Merged

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Apr 7, 2018

Launch Checklist

selection_876

same styling as the static maps api uses:

249x100 2x

  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

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.

Thanks for doing this @andrewharvey!
2 questions –
where is the SVG asset you've inlined from? Just want to make sure we're using the most up to date version and check with our design team.

is the 250 cutoff from API GL only? We're using 640px as the cutoff between compact and normal attribution, and I wonder if it makes sense to stay consistent there.

@andrewharvey
Copy link
Collaborator Author

where is the SVG asset you've inlined from? Just want to make sure we're using the most up to date version and check with our design team.

I took the current inlined svg for the normal wordmark and edited it to only contain the icon part then re inlined it. I didn't include the SVG asset since the full wordmark SVG asset isn't included currently.

It would be nice to have those inline SVGs inserted automatically from the source SVG asset, but I think we should leave that to another issue. (just opened at #6493)

is the 250 cutoff from API GL only? We're using 640px as the cutoff between compact and normal attribution, and I wonder if it makes sense to stay consistent there.

Yes it's the same as what API GL (static map api) uses. It feels right to use a smaller cutoff than the normal attribution as attribution is usually much wider to begin with.

@andrewharvey
Copy link
Collaborator Author

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks Andrew! This seems like a good change to me. I'm trying to track down someone on the product/legal side who can sign off on this and #6506. (Gotta make sure we're dotting our Is and crossing our Ts when it comes to branding and attribution.)

@@ -35,6 +36,10 @@ class LogoControl {

this._map.on('sourcedata', this._updateLogo);
this._updateLogo();

this._map.on('resize', this._updateCompact);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this binding in onRemove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, fixed.

a.mapboxgl-ctrl-logo.mapboxgl-compact {
width: 21px;
height: 21px;
background-image: svg-load('svg/mapboxgl-ctrl-logo-compact.svg');
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than adding another SVG, could we hide the wordmark portion of the existing logo SVG by toggling a CSS class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice idea but since the logo is loaded as a background-image I don't think it's possible to control that through standard CSS, I think it would need to be an embedded SVG for that to work 🤔. Given it doesn't add much weight to the file size, I'd be tempted to leave it as is

@jfirebaugh jfirebaugh merged commit cd88621 into master May 15, 2018
@jfirebaugh jfirebaugh deleted the 4282-collapsed-wordmark branch May 15, 2018 23:25
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.

collapsed Mapbox workmark on narrow maps
3 participants