Skip to content

Conversation

@TheYakuzo
Copy link

Replaces the dedicated notification component with a more general-purpose panel button.
This allows for more flexible integration of notification features and other possible features in the future.

Replaces the dedicated notification component with a more general-purpose panel button.
This allows for more flexible integration of notification features.
Addresses minor formatting inconsistencies in the code.

Enhances the MapPanelButton component for better control management
and code readability.
class NotificationControl {
constructor(eventHandler) {
class PanelButton {
constructor(eventHandler, iconClass) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we just pass two icons instead of iconClass function?

Copy link
Author

@TheYakuzo TheYakuzo Apr 6, 2025

Choose a reason for hiding this comment

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

So, set the svg directly in component parameter ?
Or just set the name of the class and I build .maplibre-ctrl-[className]-on/off in component, sound better ( But we keep the css import in this case )

Copy link
Member

Choose a reason for hiding this comment

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

Actually let's keep the function. I see that it's probably cleaner.

<MapPanelButton
enabled={eventsAvailable}
onClick={onEventsClick}
iconClass={(status) => `maplibre-ctrl-notification maplibre-ctrl-notification-${status}`}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need maplibre-ctrl-notification here, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes you're right, don't know why I put it

@TheYakuzo
Copy link
Author

TheYakuzo commented Apr 6, 2025

I rework it, let me know if I push :

// MapButton.js

import {useEffect, useMemo, useCallback} from "react";
import {map} from "../core/MapView";
import "./map-button.css";

const VALID_TYPES = ["overlay", "notification"];

class Button {
	constructor(eventHandler, type) {
		if (type && !VALID_TYPES.includes(type)) {
			console.warn(
				`Invalid button type: ${type}. Valid types are: ${VALID_TYPES.join(
					", "
				)}`
			);
		}
		this.eventHandler = eventHandler;
		this.type = type;
	}

	static getDefaultPosition() {
		return "top-right";
	}

	onAdd() {
		this.button = document.createElement("button");
		this.button.className = this.buildButtonClasses();
		this.button.type = "button";
		this.button.onclick = () => this.eventHandler(this);

		this.container = document.createElement("div");
		this.container.className = "maplibregl-ctrl maplibregl-ctrl-group";
		this.container.appendChild(this.button);

		return this.container;
	}

	onRemove() {
		this.container.parentNode.removeChild(this.container);
	}

	setEnabled(enabled) {
		this.button.className = this.buildButtonClasses(enabled);
	}

	buildButtonClasses(enabled = false) {
		const classes = ["maplibre-ctrl-icon"];

		if (this.type && VALID_TYPES.includes(this.type)) {
			classes.push(`maplibre-ctrl-${this.type}-${enabled ? "on" : "off"}`);
		}

		return classes.join(" ");
	}
}

const MapButton = ({enabled, onClick, type}) => {
	const importCSS = useCallback(async (type) => {
		if (!type || !VALID_TYPES.includes(type)) {
			return;
		}

		try {
			await import(`../${type}/${type}.css`);
		} catch (error) {
			console.error(`Failed to load CSS for ${type}:`, error);
		}
	}, []);

	const control = useMemo(() => new Button(onClick, type), [onClick, type]);

	useEffect(() => {
		importCSS(type);
	}, [type, importCSS]);

	useEffect(() => {
		map.addControl(control);
		return () => map.removeControl(control);
	}, [control]);

	useEffect(() => {
		control.setEnabled(enabled);
	}, [enabled, control]);

	return null;
};

export default MapButton;

// MainMap.jsx

 <MapButton
          enabled={overlayEnabled}
          onClick={onOverlayButtonClick}
          type="overlay"
 />
...
<MapButton
          enabled={eventsAvailable}
          onClick={onEventsClick}
          type="notification"
/>

CSS is dynamically imported into the component, but must respect the file architecture. So the two CSS files are no longer imported into MainMap.jsx.
Classes are created from the “type” passed to it

@tananaev
Copy link
Member

tananaev commented Apr 6, 2025

I don't think we need types validation, but overall I think it's fine to work like that.

@tananaev
Copy link
Member

tananaev commented Apr 6, 2025

Actually I don't like the dynamic import. I think we should stick with the original

Renames and refactors the map panel button to a more generic map button component.

This change aims to improve code reusability and clarity by providing a single, configurable button component for various map controls, instead of having a specific button tailored for panels.
@TheYakuzo
Copy link
Author

Will rebase and use this implementation on the other branch when merged

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.

2 participants