Pins: Add sidebar for pinned map cards#683
Conversation
Cloudflare Pages preview
|
Follow Up: Theme Relevant Border ColorsLatest commit addresses the feedback that the pinned entity border colors do not appear to be contrasting based on the theme. Fixed it to have different contrasting colors for the pinned centroid or territory borders based on the theme. The chosen colors are from the existing design guideline document. Also restored the map border. |
conradarcturus
left a comment
conradarcturus
left a comment
There was a problem hiding this comment.
This is looking very promising! I have a number of recommendations on styling and triggering events but it's a good start.
| @@ -32,17 +26,12 @@ type Props = { | |||
| maxWidth?: number; | |||
| }; | |||
There was a problem hiding this comment.
Maps can be in the main data view panel but also in the Details panel for Languages and Territories (click on a language and scroll to the bottom of the details and you'll see maps). It's weird to have the siderbar in those places because the page width is too small. Can you add a parameter allowSidebar default set to false and in ViewMap.tsx only set it to true.
https://translation-commons.github.io/lang-nav/data?view=Map&pinned=bel&objectID=bel
| }} | ||
| onClick={() => updatePageParams({ pinned: [] })} | ||
| > | ||
| <MapSidebar |
There was a problem hiding this comment.
only show this if allowSidebar is true
| // Floating cards are derived from the pinned page param so they can be fully restored from the | ||
| // URL after a refresh. Each card is positioned at its entity's Robinson centroid. | ||
| const floatingCards = useMemo<FloatingCard[]>(() => { | ||
| const pinnedEntities = useMemo(() => { |
There was a problem hiding this comment.
Since this is only used in MapSidebar I would remove this code from EntityMap and pass drawableEntities to MapSidebar and do this memo there.
| const pinCard = useCallback( | ||
| (entity: DrawableData) => { | ||
| updateMapScale(); | ||
| if (pinned.includes(entity.ID)) return; | ||
| updatePageParams({ pinned: [...pinned, entity.ID] }); | ||
| if (pinned.includes(entity.ID)) { | ||
| updatePageParams({ pinned: pinned.filter((id) => id !== entity.ID) }); | ||
| } else { | ||
| updatePageParams({ pinned: [...pinned, entity.ID] }); | ||
| } |
There was a problem hiding this comment.
Given the above changes, let's change this method from always being pinCard to instead be called onClick. If the sidebar is allowed, then update pins, if the sidebar is not allowed then let's just open/or update the details page: updatePageParams({objectID: entity.ID})
| position: 'relative', | ||
| background: 'var(--color-background)', | ||
| }} | ||
| onClick={() => updatePageParams({ pinned: [] })} |
There was a problem hiding this comment.
yeap, good idea. We no longer need to dismiss all pins with an off-card click.
| }); | ||
| }, [territories, getColor, isTerritoryInList, colorBy, svgLoaded]); | ||
| }, [territories, getColor, isTerritoryInList, colorBy, svgLoaded, hoveredId, pinnedIds]); | ||
|
|
There was a problem hiding this comment.
Add a separate useEffect here to update class names.
and to map.css add definitions:
.mapTerritory {
cursor: pointer;
}
.mapTerritory:hover,
.mapTerritory.hovered {
filter: brightness(1.2);
stroke: var(--color-button-primary);
stroke-width: 2px;
}
.mapTerritory.pinned {
stroke: var(--color-button-primary);
filter: brightness(1.1);
stroke-width: 2px;
}
| // Manage hovered and pinned states | |
| useEffect(() => { | |
| if (!svgLoaded) return; | |
| forEachTerritory((territory, element) => { | |
| element.classList.add('mapTerritory'); | |
| element.classList.remove('hovered'); | |
| element.classList.remove('pinned'); | |
| element.classList.remove('inList'); | |
| if (pinnedIds.includes(territory.ID)) element.classList.add('pinned'); | |
| if (hoveredId === territory.ID) element.classList.add('hovered'); | |
| }); | |
| }, [svgLoaded, hoveredId, pinnedIds]); |
| (territory: TerritoryData, element: SVGElement) => (ev: MouseEvent) => { | ||
| showHoverCard( | ||
| <div> | ||
| <strong>{territory.nameDisplay}</strong> | ||
| <div style={{ color: 'var(--color-text-secondary)' }}>Click for more</div> | ||
| </div>, | ||
| ev.clientX, | ||
| ev.clientY, | ||
| ); | ||
|
|
||
| element.style.opacity = '0.7'; |
There was a problem hiding this comment.
Let's remove the manual styling here and use CSS instead as mentioned before. Also changing from opacity to brightness.
| (territory: TerritoryData) => (ev: MouseEvent) => { | |
| showHoverCard( | |
| <div> | |
| <strong>{territory.nameDisplay}</strong> | |
| <div style={{ color: 'var(--color-text-secondary)' }}>Click for more</div> | |
| </div>, | |
| ev.clientX, | |
| ev.clientY, | |
| ); |
| const buildOnMouseLeave = useCallback( | ||
| (element: SVGElement) => () => { | ||
| onMouseLeaveTriggeringElement(); | ||
| element.style.opacity = '1'; | ||
| }, | ||
| [onMouseLeaveTriggeringElement], | ||
| ); |
There was a problem hiding this comment.
We can simplify this now that we have css
| borderRight: '1px solid var(--color-text-secondary)', | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| gap: '1em', | ||
| padding: '1em', |
There was a problem hiding this comment.
I don't think the border is necessary
| borderRight: '1px solid var(--color-text-secondary)', | |
| display: 'flex', | |
| flexDirection: 'column', | |
| gap: '1em', | |
| padding: '1em', | |
| display: 'flex', | |
| flexDirection: 'column', | |
| gap: '1em', | |
| padding: '1em', |
| setHoveredId: (id: string | null) => void; | ||
| }; | ||
|
|
||
| const MapSidebar: React.FC<Props> = ({ |
There was a problem hiding this comment.
I have other ideas on the appearance of this but I don't want to tackle too many changes at a time ;) So for now I'll call this good and we'll iterate on it after the existing feedback.
2 participants
Fixes #669
Summary: Replaces the floating map cards with a sidebar panel. Pinned entities appear in a scrollable sidebar on the left side of the map. Hovering on the said cards highlights the corresponding map centroids or territories.
Changes
Out of scope/Future work: Unit Tests, Clear All Button in sidebar, Maximum Pin Limit (Maybe).
Screenshots
Pinned Single Card
Pinned Multiple Cards
Pinned Card Hover