Discussion: mapbox controls

[korywka]

Hi. I have some controls for personal usage.
What I like about them:

1. each control have separate directory with js + css + icons
   mapbox now have 600-lines CSS
2. valid CSS / SCSS, mapbox CSS is not: https://github.com/mapbox/mapbox-gl-js/blob/master/src/css/mapbox-gl.css#L154
3. injected icons inside control button: https://github.com/bravecow/mapbox-gl-controls/blob/master/src/ruler/ruler.js#L3:

• no additional GET request for each resource
• ease of styling. we can style inline SVG with CSS.
• ease of resizing

As result:

• reduce JS bundle for mapbox
• removing svg-load plugin
• remove some GET requests for icons
• remove CSS prefixes, use autoprefixer.
• move controls from this repo to reduce package.json deps (if any)
• controls will not use any internal events or methods. it will help to understand real needs and provide better API for users (e.g. no public event for style load Events: so when the style loaded and ready to draw? #8765 for now)
• move mapbox language, mapbox draw and other controls to this monorepo. they will have same bundling tools, codestyle, docs, etc. I think it will make future support easier
• braking change

[asheemmamoowala]

@Bravecow Thank you for starting this discussion, and sharing the gl-controls repo.

We have recently had internal discussions about bundle size and plugin maintenance and are looking to incorporate some of that work into our roadmap for the rest of the year.

reduce JS bundle for mapbox

Have you tried this? What sort of size reduction were you able to achieve by removing the controls. Only the Navigation, Geolocate, Fullscreen, and Scale controls would be removable.

move mapbox language, mapbox draw and other controls to this monorepo. they will have same bundling tools, codestyle, docs, etc. I think it will make future support easier

👍

braking change

While it would be a breaking change to move the existing controls, moving the other plugins might be a good way to start this.

[korywka]

Have you tried this? What sort of size reduction were you able to achieve by removing the controls. Only the Navigation, Geolocate, Fullscreen, and Scale controls would be removable.

build-prod-min:
mapbox-gl.js: 708KB
mapbox-gl-without-controls.js: 692KB - without controls
mapbox-gl-without-all.js: 679KB - without controls, popup and marker

So -30KB of minified JS. Do not know is it a lot according to your measurements 🤷‍♂
Plus almost all CSS. Basic map CSS I think have few lines of code. To bundle all controls CSS in single file or not is another question. Splitting CSS will make smaller bundles for users who use bundles and single CSS for users who include styles with <link /> from CDN. Maybe controls styles should have both options.

[andrewharvey]

At the moment, it's possible for someone to reskin GL JS by just replacing the dist mapbox-gl.css with their own, it sounds like your suggesting embedding the CSS within the JS code, is that right? That would make re-skinning much harder?

valid CSS / SCSS, mapbox CSS is not: https://github.com/mapbox/mapbox-gl-js/blob/master/src/css/mapbox-gl.css#L154

That's because we use postcss's load-svg. The dist css should be valid, is there an issue with the sources using load-svg? The main advantage I see of using load-svg, compared to https://github.com/bravecow/mapbox-gl-controls/blob/master/src/ruler/icon-ruler.js, is we can maintain the icons as SVG files at https://github.com/mapbox/mapbox-gl-js/tree/master/src/css/svg which can be opened directly in vector graphics software, easier to inspect/open, lint, etc.

move mapbox language, mapbox draw and other controls to this monorepo. they will have same bundling tools, codestyle, docs, etc. I think it will make future support easier

Although fragmented, I'd prefer keeping Mapbox's controls in separate repos, as is currently done. It's easier to track issues and PRs releases for people who may only use a subset of the controls/plugins.

We should be able to unify the workflow regardless of separate repo or not.

remove some GET requests for icons

which icons result in extra GET requests?

[korywka]

it sounds like your suggesting embedding the CSS within the JS code, is that right?

no, only to put SVG icons inside buttons like DOM nodes. I think CSS should be a separate file for easy skinning too: https://github.com/bravecow/mapbox-gl-controls/blob/master/theme.css

The dist css should be valid, is there an issue with the sources using load-svg?

Code highlighting of CSS source file is broken in code editors.

can be opened directly in vector graphics software, easier to inspect/open, lint, etc.

You are right. I'm pretty sure converting .svg file to such export .js file is easy for rollup. It will look something like that:

import rulerIcon from './ruler-icon.svg';

button.appendChild(rulerIcon());

I'd prefer keeping Mapbox's controls in separate repos, as is currently done. It's easier to track issues and PRs releases for people who may only use a subset of the controls/plugins

If some user have an idea for a new control, a new project should be made from the very beginning (in case there is not any mapbox-plugin-template repos). With monorepo users would just make pull request with their control's code. Also, I hope, it will be easier to support all plugins to work with latest mapbox-gj-js version. You know that there are some controls / plugins that don't work with latest mapbox streets styles, for example. peerDependencies will be common for the repo and it will push maintainers to update all plugins.

which icons result in extra GET requests?

Ah, I forgot that icons are inlined in CSS file as base64 here. Thanks, there is no extra GET request 👍

We should be able to unify the workflow regardless of separate repo or not.

Absolutely. Don't get me wrong, I'm not trying to tell how cool I am here or any other kind of offense. I just see that controls / plugins is something that should be improved and trying to help. Yesterday my customized GeolocationControl was broken cause of #8367. It will be more obvious for users and easier to customize if all icons are centered inside buttons by the same way. For now just this single control doesn't work with display: flex

[korywka]

@andrewharvey struck out about additional GET requests 🙄

[korywka]

can be opened directly in vector graphics software, easier to inspect/open, lint, etc.

Already done: https://github.com/bravecow/mapbox-gl-controls/blob/master/src/compass/icon-pointer.svg

Wrote simple rollup converter: https://github.com/bravecow/rollup-inline-svg

[andrewharvey]

no, only to put SVG icons inside buttons like DOM nodes. I think CSS should be a separate file for easy skinning too: https://github.com/bravecow/mapbox-gl-controls/blob/master/theme.css

oh right, so instead of a background-image which is base64 svg, it would just be the raw SVG inside the , so not styled with CSS?

If some user have an idea for a new control, a new project should be made from the very beginning
(in case there is not any mapbox-plugin-template repos).

A template repo would be useful.

[korywka]

oh right, so instead of a background-image which is base64 svg, it would just be the raw SVG inside the , so not styled with CSS?

nope. only new Control({icon: '<svg>....</svg>'}). CSS base64 background cool if you treat all icons as monolith. Inline gives you flexibility for partial coloring, partial scaling and animation. I understand that this is a very controversial issue, each has its own pros and cons. And maybe inlining too much for this and "monolith icon" is quite enough.

For example, this one:

Would be something like this:

.mapboxgl-ctrl-icon.mapboxgl-ctrl-geolocate svg {
    fill: #333;
}


.mapboxgl-ctrl-icon.mapboxgl-ctrl-geolocate:disabled svg {
    fill: #aaa;
}

.mapboxgl-ctrl-icon.mapboxgl-ctrl-geolocate.mapboxgl-ctrl-geolocate-active svg{
    fill: #33b5e5;
}

.mapboxgl-ctrl-icon.mapboxgl-ctrl-geolocate.mapboxgl-ctrl-geolocate-active-error svg{
    fill: #e58978;
}

So does inlining makes changing icons hard? Do not think so. It just moves to JS instead of CSS.
Does inlining makes coloring easy? I think - definitely.

[korywka]

FYI: Rollup moves all official plugins to monorepo:

https://github.com/rollup/plugins

[korywka]

After ~4 years my suggestion slightly changed to the side as it does rollup plugins: monorepo + pnpm. But still think it is a good idea to remove controls from core and structure them (e.g. 'official' draw control and fullscreen control have different icon size and color). Especially now, when a new major release is coming.