Refactor/display feature count on cluster markers - #932
Conversation
…ved double setStyle
|
…ture-count-on-cluster-markers
|
🕊️ @dopenguin |
dopenguin
left a comment
dopenguin
left a comment
There was a problem hiding this comment.
Partial review, looking at src/core/utils/markerSVG.ts up next
| count: number, | ||
| displayFeatureCount: boolean | ||
| ) => Style | ||
| export type GetSVGConfigFunction = (digits: string) => MarkerSVGConfig |
There was a problem hiding this comment.
The type is redundant as the information can be inferred.
There was a problem hiding this comment.
Typing ensures that this function always returns an MarkerSVGConfig-object. Should the function typing be completely removed? Or do you have something different in mind?
There was a problem hiding this comment.
dopenguin likes them completely removed while they're inferrable. I personally disagree with the same argument you provided: It will, on later changes, make sure that there's no accidental type change due to an oversight.
IMO you may decide either way. If you keep it, you may just resolve this conversation. 🎩
| ) => Style | ||
| export type GetSVGConfigFunction = (digits: string) => MarkerSVGConfig | ||
| export type GetTextPositionFunction = (path: string) => TextPosition | ||
| export type PinShape = 'circle' | 'pill' |
There was a problem hiding this comment.
As this is not used anywhere else, this can just directly be set on MarkerSVGConfig.
Also, do you think there will be a third option? Otherwise, using a boolean with a different name seems easier.
There was a problem hiding this comment.
Yes I thought, maybe there will be more different markers, like rectangle ones or square ones.
There was a problem hiding this comment.
I see no need to change this, any change would be arbitrary. 🎩
| * `width` and `height` of the `<svg>`-cluster-marker. | ||
| * | ||
| * @defaultValue `[40, 36]` | ||
| * @defaultValue `[40 * 2, 36 * 2]` |
There was a problem hiding this comment.
Why not 80 and 72? If changed, this should be done on both types and all other instances.
There was a problem hiding this comment.
Because 40 and 36 were the smallest values that worked well, I thought it would make sense to keep them and scale them up.
There was a problem hiding this comment.
Makes sense to me. Please add that reason to the comment so that the next person reading knows; it's a nice information to have for future modification/configurability.
| }" viewBox="0 0 30 43" xmlns="http://www.w3.org/2000/svg"> | ||
| const makeMarker = ({ fill, size, stroke }: MarkerStyle) => | ||
| `${prefix}${encodeSVG(` | ||
| <svg fill="none" width="${size[0]}" height="${size[1]}" viewBox="${circlePin.viewBox}" xmlns="http://www.w3.org/2000/svg"> |
There was a problem hiding this comment.
I copied this code from the SVG that the UX Designer provided.
There was a problem hiding this comment.
For fun, and by fun I mean science since messing around and writing down the results qualifies, I set the stroke to ''.
Then, I removed fill="none" on the root node.
It improved a little! ... maybe? The other one is a little futuristic, I'd say! Looks like many shapes just default to black fill when an empty string is given: https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Attribute/fill#circle
However, stroke is not intended to be optional, so I consider it fine to keep fill="none" if that's what we were sent. Especially since any new changes/deliveries will contain it again, anyway, and I don't think this should lead to any headache down the line.
🎩
| displayFeatureCount: boolean, | ||
| svgConfig: MarkerSVGConfig | ||
| ) => { | ||
| return svgConfig.pinShape === 'circle' |
There was a problem hiding this comment.
If a const is used, then one should gently drop the return
There was a problem hiding this comment.
so you would rather implement an if-statement? Oder do you have something different in mind?
If we put the decisioin wether to display a circle or pill in the returned string, we will lose a huge amount of performance.
There was a problem hiding this comment.
He probably wants this pattern
const f = () => a ? 'b' : 'c'
instead of
const f = () => { return a ? 'b' : 'c' }
.
🎩 since it doesn't really matter.
| const memoizeStyle = (getMarker: GetMarkerFunction): GetMarkerFunction => { | ||
| const memoizedCountStyle = memoCountStyle(getMarker) | ||
| const memoizedStyle = memoStyle(getMarker) | ||
| return (style, count, displayFeatureCount) => | ||
| displayFeatureCount | ||
| ? memoizedCountStyle(style, count, displayFeatureCount) | ||
| : memoizedStyle(style, count, displayFeatureCount) | ||
| } |
There was a problem hiding this comment.
| const memoizeStyle = (getMarker: GetMarkerFunction): GetMarkerFunction => { | |
| const memoizedCountStyle = memoCountStyle(getMarker) | |
| const memoizedStyle = memoStyle(getMarker) | |
| return (style, count, displayFeatureCount) => | |
| displayFeatureCount | |
| ? memoizedCountStyle(style, count, displayFeatureCount) | |
| : memoizedStyle(style, count, displayFeatureCount) | |
| } | |
| const memoizeStyle = | |
| (getMarker: GetMarkerFunction): GetMarkerFunction => | |
| (style, count, displayFeatureCount) => | |
| displayFeatureCount | |
| ? memoCountStyle(getMarker)(style, count, displayFeatureCount) | |
| : memoStyle(getMarker)(style, count, displayFeatureCount) |
Or did you already test this and this caused issues?
There was a problem hiding this comment.
🏌️﹏෴﹏෴﹏෴﹏෴﹏෴﹏෴﹏⛳, but also 🎩.
| const memoCountStyle = (getMarker: GetMarkerFunction): GetMarkerFunction => { | ||
| const countCache = new Map<number, Map<MarkerStyle, Style>>() | ||
| return (style, count, displayFeatureCount) => { | ||
| // vielleicht auslagern und ind warnMeoLeak einbinden? |
There was a problem hiding this comment.
The comment is no longer present. Whatever's been decided has been decided. 🎩
| // vielleicht auslagern und ind warnMeoLeak einbinden? | ||
| const getTotalCachedStyles = ( | ||
| countCache: Map<number, Map<MarkerStyle, Style>> | ||
| ): number => { |
There was a problem hiding this comment.
| ): number => { | |
| ) => { |
There was a problem hiding this comment.
Why would wen want tot drop the return-type? As soon as someone would work at this function, it is clear, that a number is expected and changing the return-type could probably effect other parts of the code.
There was a problem hiding this comment.
I agree @dMapybara.
However, the function no longer exists anyway. 🎩
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
27a8652 to
8c119b7
Compare
There was a problem hiding this comment.
I feel like markers should receive its own folder like export with src/core/utils/markers.ts being the index-file.
Then, this file may be split in multiple files as well such that all the different MarkerSVGConfigs receive their own files.
There was a problem hiding this comment.
I agree since markers.ts and markerSVG.ts belong together, i.e. markersSVG.ts isn't used outside of markers.ts and probably never will be.
I am indifferent about splitting the files further since marker.ts isn't that long and markerSVG.ts has a low complexity.
| y: parseFloat(centerYStr), | ||
| } | ||
| } | ||
| // The paths always follow the scheme: scheme: M[x0] [y0]C[x1] [y1] [x2] [y2] [x3] [y3]S[x4] [y4] [x5] [y5] [x6] [y6] [x7] [y7]Z |
There was a problem hiding this comment.
I would have no idea what to do with this information tbh. If I were, I'd expect this comment to be set somewhere on MarkerSVGConfig. Especially as it is also added twice in this file
There was a problem hiding this comment.
If the path in the SVG deviates from this scheme, the marker may no longer be displayed correctly. Furthermore, the text positivity is no longer determined correctly.
It ist relevant if you want to implement a different SVG as a Marker - you would have to adjust the calculation as well.
But this comment should be above the calculating-function.
Therefore I would suggest to move it there.
There was a problem hiding this comment.
There's a very accepting dopenguin thumb on that comment, so I assume that's the solution agreed upon.
There was a problem hiding this comment.
Worker bee @warm-coolguy dances the following information to worker bee @dMapybara:
- There are unresolved conflicts regarding the target branch that have to be resolved in order to progress.
- I have proceeded to close a lot of threads that seem resolved. On those with unclear status, a comment has been added regarding next plausible steps.
- I remembered that you remembered me that worker bee @dopenguin danced us a message regarding a greater cluster distance and smaller markers. I have prepared the following proposal: proposal.patch In this, the snowbox has been configured to have markers slightly smaller than before, and a cluster distance way greater than before. However, these values are arbitrary. Please adjust as you deem fit. I assume the goal is to have a nice snowbox setup for the Meldemichel report clusters where they don't overlap too much.
[/dance]
3 participants
Summary
Summarization of the changes in a short and concise sentence.
The main change is the display of the number of features in a cluster. This can be configured via index.ts.
Instructions for local reproduction and review
npm inpm run snowboxPull Request Checklist (for Assignee)
UI has been tested in the following tools regarding accessibility (only regarding functionality affected in this PR)
Relevant tickets, issues, et cetera