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

highlight selected feature #1359

Merged

Conversation

jschleic
Copy link
Contributor

@jschleic jschleic commented Oct 5, 2023

I'd like to suggest some more interactive highlighting of paths. What do you think about:

Bildschirmaufzeichnung.vom.2023-10-05.21-19-23.webm

A very simple implementation uses the popup events to increase opacity and weight.
On maps with multiple lines one can see much easier, where the line starts and ends.
This implementation also highlights the lines of polygons.

Do you think, such a feature is feasible?
Should parameters be configurable?

when opening a popup increase opacity and weight.
In maps with multiple lines one can see much easier, where the line
starts and ends.
@yohanboniface
Copy link
Member

Nice proposal, thanks! 👍

I don't think we need this to be configurable at this stage.

I wonder if we can release this without having an highlight for markers to (remember that the "popup" can be a side panel, o no directly associated to the related feature), and in this case how to deal with those, maybe touching the shadow ? I guess the answer will be different for each type of marker (may be done in CSS, after adding a class in JS).

For polygons, maybe also increase fill opacity ? In some situations polygons may have no border.

cc @Aurelie-Jallut @davidbgk

cf #334

@Aurelie-Jallut
Copy link

In consultation I want to differentiate the point / zone / line activated.e

  • enlarge markers
  • Add contour to line
  • Add contour to polygon (and possibly increase opacity)

@yohanboniface
Copy link
Member

In consultation I want to differentiate the point / zone / line activated.e

That's the point of this proposal :)

enlarge markers

That's an option, agreed

Add contour to line

Are you then excluding the current proposal, which enlarge the line when selected ?

@Aurelie-Jallut
Copy link

I was under the impression that only the opacity changed, but yes, it's a good idea to make the line bigger, which would mean putting an outline in the same color.

@jschleic
Copy link
Contributor Author

jschleic commented Oct 6, 2023

Thank you very much for the quick feedback!

I don't see how an outline in a different color around a path could be implemented. Except adding the same path again behind the first one.

Regarding markers: Does the following mock-up go into the direction you were thinking of @Aurelie-Jallut ?

Bildschirmaufzeichnung.vom.2023-10-06.22-28-10.webm

I will continue on this as time allows...

@yohanboniface
Copy link
Member

I don't see how an outline in a different color around a path could be implemented. Except adding the same path again behind the first one.

Increasing weight is fine!

Regarding markers: Does the following mock-up go into the direction you were thinking of @Aurelie-Jallut ?

Sounds good to me! But usually markers have full opacity, so I think we'll need increasing the shape for all, or maybe playing with the shadow (bigger / changing color / moving it away from the marker so it appears like raised…)?

Thanks again for your work, much appreciated :)

add .umap-icon-active class for currently selected marker icon.
CSS styles override the marker styles to display
* in the foreground
* with full opacity
* with larger sizes where possible (ball and circle)
to also highlight path and markers when using the side panel.
Therefore we duplicate the event code from the base class.
@jschleic jschleic marked this pull request as draft October 10, 2023 08:34
sqrt gives larger increase for low opacities like 0.2
while preserving some (tiny) shine-through for large values like 0.8
and is limited to the desired range of (0,1]
@davidbgk
Copy link
Contributor

Sounds good to me! But usually markers have full opacity, so I think we'll need increasing the shape for all, or maybe playing with the shadow (bigger / changing color / moving it away from the marker so it appears like raised…)?

Maybe we could reduce the opacity of all other paths/points when only one is selected/hovered to make it stand out?

@yohanboniface
Copy link
Member

yohanboniface commented Oct 10, 2023

Maybe we could reduce the opacity of all other paths/points when only one is selected/hovered to make it stand out?

This can be a lot of paths/points, not sure we want this extra computation (and DOM change because of the redraw) :/

@jschleic
Copy link
Contributor Author

One issue is still open:

After adding a marker in edit mode and switching back to display mode the marker highlight doesn't work any longer. The highlight callback is called and the css class umap-icon-active is added in the debugger but not in DOM?! Any ideas @yohanboniface ?
grafik
grafik

options.icon pointer is invalid after hide() show() cycle.
@jschleic
Copy link
Contributor Author

jschleic commented Oct 19, 2023

After adding a marker in edit mode and switching back to display mode the marker highlight doesn't work any longer.

The culprit seems to be this.map.removeLayer(this.layer) triggered during save():

this.redraw() // Needed for reordering features

A manual hide() show() cycle in the layer box has the same effect.

Thus I have to keep the highlight callback in the marker class (cf. 88746d5)

therefore setting opacity of the example path to 0.6
@jschleic jschleic marked this pull request as ready for review October 19, 2023 19:34
@yohanboniface
Copy link
Member

Thanks a lot! I'm travelling now, I'll have a look during the week-end :)

@yohanboniface
Copy link
Member

So, I've had a look and I'm fine with the logic :)

I'm not totally convinced by the rendering part. I'm making a summary of what's actually implemented in the PR, so maybe @Aurelie-Jallut can have an overview and help us on the finishing part :)

Path:

Sounds good to me!

Screencast.from.2023-10-21.12-20-05.webm

Polygon:

Sounds good to me!

Screencast.from.2023-10-21.12-20-28.webm

Circle:

Sounds good to me!

Screencast.from.2023-10-21.12-20-46.webm

Default:

Shadow becomes white, which does not work on this white background.

Screencast.from.2023-10-21.12-23-33.webm

I've tried to play with multiple shadow, but anything I can do is damn ugly, eg.:

Screencast.from.2023-10-22.10-37-05.webm

Drop:

Same white shadow issue.

Screencast.from.2023-10-21.12-22-35.webm

Pin:

The ball grows but it becomes a bit square. If we get to a good working shadow highlight for Default and Drop, maybe we could use the same here, and there will be no need for increasing the ball size ?

Screencast.from.2023-10-21.12-21-47.webm

@jschleic
Copy link
Contributor Author

Square'ish Pin radius fixed, thanks!

What about following the "larger"-strategy for Drop and Default as well? That would look like:

Bildschirmaufzeichnung.vom.2023-10-22.20-42-32.webm

For Default and Drop markers I would suggest to change to svg path from the composite container+arrow anyways. Semi-opaque markers look composite with circle+arrow. An svg would then allow an easier outline border around the marker as @Aurelie-Jallut suggested in #1359 (comment) - but that's imho a separate PR.

Another option is to decrease the default marker opacity. To make highlighted markers with opacity: 1 stand out. That would of course only work for new maps.

P.S. Are there possibly old maps out there without any opacity set (neither in the map options nor the path)?? The test case data has such null polygon opacity resulting in an invisible polygon on click with this PR applied - is that an issue we should add an if(opacity)?

@yohanboniface
Copy link
Member

What about following the "larger"-strategy for Drop and Default as well?

Makes sense! Let me check with Aurelie before putting more energy. Hopefully this Friday (we had a big event last week, that explains our time for synchronizing and giving you useful feedback, and I'm sorry for that).

For Default and Drop markers I would suggest to change to svg path from the composite container+arrow anyways.

Why not, but a few questions:

  • could we add a shadow on a path ?
  • could we easily integrate the pictogram/letter ?
  • what about rendering performance (this HTML/CSS version has quite poor performance, and we should makes this better at some point, hence my question :) )

P.S. Are there possibly old maps out there without any opacity set

Honestly I'm not sure about this point, so I'd say let's be more defensive just in case :)

Thanks again for your work!

@yohanboniface
Copy link
Member

@jschleic I just discussed with Aurélie and she suggests we should use same pattern for all, which is increasing the shape, so:

  • path: increase the weight and opacity, as you did
  • polygon: same
  • circle: same
  • pin: increase the ball and the shadow (but without changing the colour)
  • drop: increase the shape, and shadow, and opacity when it's not already 1
  • default: same

Does that make sense for you ? Do you still have time and energy to continue the work ? I think we are really close the final point, but feel free!

Thanks again :)

@jschleic
Copy link
Contributor Author

jschleic commented Oct 27, 2023 via email

@yohanboniface
Copy link
Member

to change to svg path from the composite container+arrow anyways.

Just found this, so pasting it here for later use: http://iatkin.github.io/leaflet-svgicon/

* drop and div marker increased to 36px size
* adjust drop arrow accordingly
* black box-shadow with increased spread
'null' is not a sensible default
@@ -161,7 +161,7 @@ L.Util.detectFileType = (f) => {
}

L.Util.usableOption = (options, option) =>
options[option] !== undefined && options[option] !== ''
Copy link
Contributor Author

@jschleic jschleic Nov 1, 2023

Choose a reason for hiding this comment

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

I had to exclude null here, to make getDynamicOption() return a default number. I tested with the test case data from _pre.js. Is there any other special meaning for null, that should be usable?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@jschleic jschleic Nov 14, 2023

Choose a reason for hiding this comment

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

Right - thanks! Then I think null opacity as used in the testcase is not an actually valid setting used in real maps??! See 34bb2d6 for another try to eliminate that special case.

@yohanboniface
Copy link
Member

Another discussion about using SVG markers that may be of interest: Leaflet/Leaflet#8766

partly revert 952385 - since ´null´ is actually used in ternary fields.
Remove null opacity from the testcase. It should be either unset or a
numeric value.
@yohanboniface yohanboniface changed the title highlight selected path highlight selected feature Nov 15, 2023
@yohanboniface yohanboniface merged commit 4b8b0b9 into umap-project:master Nov 15, 2023
@yohanboniface
Copy link
Member

Very cool work, thanks a lot! 🚀

yohanboniface added a commit that referenced this pull request Feb 4, 2024
This can happen in a situation where:
- a layer as zoom limits
- we click on a feature, which opens a popup
- then zoom over the layer's limit (with
  scroll or keyboard, not keyboard, in order to not close the popup)
- then click in anywhere in the map, which will close the popup

Since the highlight of features on click (cf #1359), we redraw them
on popupclose, which explains the bug described above.

fix #1575
@dkbg
Copy link

dkbg commented Feb 29, 2024

Thanks for implementing the feature. One issue I've noticed is that it can often be difficult to differentiate between highlighted and non-highlighted markers. I came here searching for whether this had been implemented, I thought that the version of uMap I was using didn't have the feature, not realising that the highlighting feature was there, just not at all obvious.

Here's a screenshot for reference. One of these markers is highlighted, but it isn't obvious which one.
image

@jschleic
Copy link
Contributor Author

@dkbg Thanks for your feedback!

As discussed above, adding a colored countour, would imho require implementation of svg markers. That would be a rather big efford to re-implement all the marker classes. Any help welcome :-)

For now you could try to reduce the default icon opacity to something like 0.6 - that makes the active marker stand out like in the video above: #1359 (comment)

@clwacker
Copy link

Thanks for the tip.
But changing the opacity has little effect when you choose circles to represent the points on the map.

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.

None yet

6 participants