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.

Sign up for GitHub

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

Jump to bottom

Use inline SVGs and refresh CSS #8884

Closed
wants to merge 2 commits into from
Closed

Use inline SVGs and refresh CSS #8884

wants to merge 2 commits into from

Conversation

kkaefer
Copy link
Contributor

@kkaefer kkaefer commented Oct 21, 2019

Supersedes #8874, fixes #5784. Implements #8767.

We now insert SVG nodes directly into the DOM for displaying all icons and buttons instead of using data URLs in the CSS file. This enables us to restyle them more easily, e.g. for dark mode or high-contrast mode. Also reduces the size of some of the SVG icons significantly and changes the Mapbox logo to a version that is aligned better to the pixel grid on low-res devices.

@kkaefer
Copy link
Contributor Author

kkaefer commented Oct 21, 2019

This changes the way we render icons significantly, so we need some extensive testing in all browsers. It's also a breaking change for users who are customizing the controls with CSS. While all of the classes are still there, the elements will now have svg child nodes with the actual icon. Users can either hide those, or instantiate the *Controls with custom SVGs.

@kkaefer
Copy link
Contributor Author

kkaefer commented Oct 21, 2019

Before/After:

out

Attribution Before/After:

attribution

IE 11 Black Mode:
high-contrast-black

IE 11 White Mode:
high-contrast-white

@kkaefer
Use SVG in the DOM instead of as background image
4e412ac 
We now insert SVG nodes directly into the DOM for displaying all icons and buttons instead of using data URLs in the CSS file. This enables us to restyle them more easily, e.g. for dark mode or high-contrast mode. Also reduces the size of some of the SVG icons significantly and changes the Mapbox logo to a version that is aligned better to the pixel grid on low-res devices.
@mourner
Copy link
Member

mourner commented Oct 21, 2019

Heads up that this PR will require an update on GL Native side which currently uses the Node-specific shaders entry point (if this didn't change recently).

@andrewharvey
Copy link
Collaborator

cc @Bravecow

@andrewharvey
Copy link
Collaborator

andrewharvey commented Oct 22, 2019 •
edited
Loading

I would have thought keeping everything in a CSS file would have been better, that way it's easy for someone to either supply their own CSS to override certain classes to provide their own theme, or just drop in replace the provided CSS.

What are the changes to shaders? How does this relate to the themeing?

I have concerns also with the way SVG's can now be provided as arguments to the controls, some of these like the FullScreen and Geolocate controls are now expecting certain element IDs or Classes in the SVG, but none of that is documented in the public facing API docs. Currently (before this PR) you'd just go through the CSS and you'd see which classes you need to style, but now without that, there's no documentation for what the custom SVG must look like.

@korywka
Copy link
Contributor

korywka commented Oct 22, 2019 •
edited
Loading

SVG inline 👍 but for 4 icons you call new window.DOMParser()....

For the same reason for mapbox-gl-controls I created https://github.com/bravecow/rollup-inline-svg that returns SVG DOM Node using window.DOMParser(). (didn't find something like this among all SVG rollup plugins...)

korywka
korywka reviewed Oct 22, 2019
View reviewed changes
src/ui/control/logo_icon.svg
<path id="text" d="M50.63 8c.13 0 .23.1.23.23V9c.7-.76 1.7-1.18 2.73-1.18 2.17 0 3.95 1.85 3.95 4.17s-1.77 4.19-3.94 4.19c-1.04 0-2.03-.43-2.74-1.18v3.77c0 .13-.1.23-.23.23h-1.4c-.13 0-.23-.1-.23-.23V8.23c0-.12.1-.23.23-.23h1.4zm-3.86.01c.01 0 .01 0 .01-.01.13 0 .22.1.22.22v7.55c0 .12-.1.23-.23.23h-1.4c-.13 0-.23-.1-.23-.23V15c-.7.76-1.69 1.19-2.73 1.19-2.17 0-3.94-1.87-3.94-4.19 0-2.32 1.77-4.19 3.94-4.19 1.03 0 2.02.43 2.73 1.18v-.75c0-.12.1-.23.23-.23h1.4zm26.375-.19a4.24 4.24 0 00-4.16 3.29c-.13.59-.13 1.19 0 1.77a4.233 4.233 0 004.17 3.3c2.35 0 4.26-1.87 4.26-4.19 0-2.32-1.9-4.17-4.27-4.17zM60.63 5c.13 0 .23.1.23.23v3.76c.7-.76 1.7-1.18 2.73-1.18 1.88 0 3.45 1.4 3.84 3.28.13.59.13 1.2 0 1.8-.39 1.88-1.96 3.29-3.84 3.29-1.03 0-2.02-.43-2.73-1.18v.77c0 .12-.1.23-.23.23h-1.4c-.13 0-.23-.1-.23-.23V5.23c0-.12.1-.23.23-.23h1.4zm-34 11h-1.4c-.13 0-.23-.11-.23-.23V8.22c.01-.13.1-.22.23-.22h1.4c.13 0 .22.11.23.22v.68c.5-.68 1.3-1.09 2.16-1.1h.03c1.09 0 2.09.6 2.6 1.55.45-.95 1.4-1.55 2.44-1.56 1.62 0 2.93 1.25 2.9 2.78l.03 5.2c0 .13-.1.23-.23.23h-1.41c-.13 0-.23-.11-.23-.23v-4.59c0-.98-.74-1.71-1.62-1.71-.8 0-1.46.7-1.59 1.62l.01 4.68c0 .13-.11.23-.23.23h-1.41c-.13 0-.23-.11-.23-.23v-4.59c0-.98-.74-1.71-1.62-1.71-.85 0-1.54.79-1.6 1.8v4.5c0 .13-.1.23-.23.23zm53.615 0h-1.61c-.04 0-.08-.01-.12-.03-.09-.06-.13-.19-.06-.28l2.43-3.71-2.39-3.65a.213.213 0 01-.03-.12c0-.12.09-.21.21-.21h1.61c.13 0 .24.06.3.17l1.41 2.37 1.4-2.37a.34.34 0 01.3-.17h1.6c.04 0 .08.01.12.03.09.06.13.19.06.28l-2.37 3.65 2.43 3.7c0 .05.01.09.01.13 0 .12-.09.21-.21.21h-1.61c-.13 0-.24-.06-.3-.17l-1.44-2.42-1.44 2.42a.34.34 0 01-.3.17zm-7.12-1.49c-1.33 0-2.42-1.12-2.42-2.51 0-1.39 1.08-2.52 2.42-2.52 1.33 0 2.42 1.12 2.42 2.51 0 1.39-1.08 2.51-2.42 2.52zm-19.865 0c-1.32 0-2.39-1.11-2.42-2.48v-.07c.02-1.38 1.09-2.49 2.4-2.49 1.32 0 2.41 1.12 2.41 2.51 0 1.39-1.07 2.52-2.39 2.53zm-8.11-2.48c-.01 1.37-1.09 2.47-2.41 2.47s-2.42-1.12-2.42-2.51c0-1.39 1.08-2.52 2.4-2.52 1.33 0 2.39 1.11 2.41 2.48l.02.08zm18.12 2.47c-1.32 0-2.39-1.11-2.41-2.48v-.06c.02-1.38 1.09-2.48 2.41-2.48s2.42 1.12 2.42 2.51c0 1.39-1.09 2.51-2.42 2.51z" />
</defs>
<style>
.mapboxgl-ctrl-logo-outline {
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 it is better to move styles to CSS (so they could be minified, autoprefixed, ...) or SVG attributes.

korywka
korywka reviewed Oct 22, 2019
View reviewed changes
src/ui/control/fullscreen_icon.svg
@@ -0,0 +1,18 @@
<svg viewBox="0 0 29 29" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
<defs>
<path id="expand" d="M7.5 5C5.75 5 5 5.75 5 7.5V13h1l1.5-3 5.5 4 1-1-4-5.5L13 6V5H7.5z"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

If we inline SVG, we should care about unique id(as they are just plain nodes) attributes: https://stackoverflow.com/a/37000951/643514 (https://css-tricks.com/youre-inlining-svg-icons-deal-unique-titles-ids/)

I suggest to prefix them with mapbox- at least. (in case it is not already done by svgo or something).

korywka
korywka reviewed Oct 22, 2019
View reviewed changes
src/css/mapbox-gl.css
@@ -85,8 +85,8 @@
}
Copy link
Contributor

@korywka korywka Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Since postcss-nested is not valid syntax for CSS, I suggest to change extension to .scss to save code highlighting in code editors.

In future using postcss-import you can simplify this CSS like this one. 😉

@kkaefer kkaefer mentioned this pull request Oct 23, 2019
Merged
@kkaefer
Copy link
Contributor Author

kkaefer commented Oct 29, 2019

Closing in favor of #8874

@kkaefer kkaefer closed this Oct 29, 2019
@kkaefer kkaefer deleted the inline-svg branch October 29, 2019 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@korywka korywka korywka left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Map navigation controls not visible in windows high contrast mode
4 participants
@kkaefer @mourner @andrewharvey @korywka