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

Refactoring HTML anatomy of items in layers #531

Merged
merged 22 commits into from
Oct 28, 2021

Conversation

Anshpreet8
Copy link
Contributor

@Anshpreet8 Anshpreet8 commented Oct 8, 2021

Used the svg "More Vert" icon instead of the details element to avoid having to nest interactive elements inside the summary element, as the suggested layout in the proposal for alternative layer item anatomy

Essentially the same PR as #530, which was closed due to renaming the branch to refactoringLayer

List of Issues that can be closed from this PR:
Closes #263

@ahmadayubi
Copy link
Member

@Malvoz was there a specific reason for using SVG icons rather than HTML icons? Like the ones used for the zoom, reload and full screen buttons?

@Malvoz
Copy link
Member

Malvoz commented Oct 8, 2021

@ahmadayubi

HTML characters can be problematic because they tend to render differently depending on the browser/OS.

They're also much more likely to be unintentionally affected by author-provided styles such as font-size and letter-spacing, which is why we reset styles (before leaflet.css and mapml.css is applied). See for example how the zoom controls look with a 5px letter-spacing in #140.

@ahmadayubi
Copy link
Member

@Malvoz That makes sense but how differently could the x symbol, for example, render across browsers, it would seem like a pretty consistent symbol.

We used it in the past, and leaflet uses them too, I wonder if it would be better to stick to using them simply for consistency. Also having to generate SVG in JavaScript isn't the most developer friendly code to read either.

@Malvoz
Copy link
Member

Malvoz commented Oct 12, 2021

@ahmadayubi In a nutshell SVG icons generally look better and seem less problematic, but if you'd rather use HTML characters that's fine.

having to generate SVG in JavaScript isn't the most developer friendly code to read

If we want to allow customization of icons in controls at some point, we may perhaps want them to be CSS background-images, such that authors can override them, using e.g. ::part(reload-button) { background-image: ... } (depends on #497). But maybe that can be changed later, as part of or after #19 (in which we'll have to specify icons for all controls, I think).

@Malvoz Malvoz mentioned this pull request Oct 15, 2021
7 tasks
@Anshpreet8 Anshpreet8 marked this pull request as ready for review October 18, 2021 21:03
src/mapml.css Outdated Show resolved Hide resolved
src/mapml.css Outdated Show resolved Hide resolved
src/mapml.css Outdated Show resolved Hide resolved
Copy link
Member

@ahmadayubi ahmadayubi left a comment

Choose a reason for hiding this comment

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

I didn't run into any issues, I did notice some alignment issues:

inline

It may also be nice to add interaction to the whitespace:
interaction

Previously it would expand the former details element.

Lastly, (not that important), the icons & interaction points seem inconsistent and have different sizes, although I don't think this can be changed given the current requirements and restrictions set (i.e. buttons being 44px and html details element icon being smaller):
icons

src/mapml/layers/MapLayer.js Outdated Show resolved Hide resolved
src/mapml/layers/MapLayer.js Outdated Show resolved Hide resolved
src/mapml.css Outdated Show resolved Hide resolved
src/mapml.css Outdated Show resolved Hide resolved
Copy link
Member

@Malvoz Malvoz left a comment

Choose a reason for hiding this comment

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

It's possible to drag to re-order layers in the control when dragging starts on the layer name and the "Layer Settings" control, but not the "Remove Layer" control, just noting that inconsistency.

@@ -481,66 +481,96 @@ export var MapMLLayer = L.Layer.extend({
return this.options.attribution;
},
getLayerUserControlsHTML: function () {
var fieldset = document.createElement('fieldset'),
var fieldset = L.DomUtil.create('fieldset', 'mapml-layer-item'),
input = document.createElement('input'),
Copy link
Member

Choose a reason for hiding this comment

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

We should probably use L.DomUtil.create(...) consistently. I think you can call it with no class values and have it work like document.createElement().

@prushforth
Copy link
Member

prushforth commented Oct 27, 2021

Everything looks very good to me, and it works very well, too. Good work @Anshpreet8 ! Thanks @Malvoz and @ahmadayubi for your help and reviews. I will merge once Ahmad signs off on it.

Copy link
Member

@ahmadayubi ahmadayubi left a comment

Choose a reason for hiding this comment

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

It looks good to me, there are some parts of the code that still use document.createElement from before @Anshpreet8 's changes, if you'd like @Anshpreet8 you could convert all old bits to use L.DomUtil.create() as well to update the entire function and make it more consistent. You could also take advantage of L.DomUtil.create()'s ability to append child and add classes in a single line of code.

I marked out a few places that could be changed but there are other places as well. But these are not critical changes so @prushforth feel free to merge without these changes if you'd like.

src/mapml/layers/MapLayer.js Outdated Show resolved Hide resolved
src/mapml/layers/MapLayer.js Outdated Show resolved Hide resolved
src/mapml/layers/MapLayer.js Show resolved Hide resolved
src/mapml/layers/MapLayer.js Outdated Show resolved Hide resolved
@Anshpreet8
Copy link
Contributor Author

Adding a container to append to using L.DomUtil.create without including a classname present does not append it, so I've used appendChild for those scenarios.

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.

HTML anatomy of items in the layers list needs refactoring
4 participants