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

ui-charts: make picking elements modular #963

Merged
merged 4 commits into from
Mar 9, 2025
Merged

Conversation

emersion
Copy link
Member

@emersion emersion commented Mar 7, 2025

Currently, PickingElement is a closed type: only 3 types explicitly listed inside osrd-ui are accepted. External layers can't define their own picking elements. For instance, an external user with a custom work schedule layer cannot define a work schedule element to display a tooltip on hover.

Fix this by making PickingElement an open-ended type: it only has a type field, rest of the fields are layer-specific. Each layer defines the more specific type and provides a helper to convert to that more specific type.

(I've contemplated using generics instead, but they spread all over the place and make everything more complicated.)

See individual commits.

Closes #958

@emersion emersion requested review from Synar and clarani March 7, 2025 12:11
@emersion emersion requested a review from a team as a code owner March 7, 2025 12:11
@emersion emersion force-pushed the emr/modular-picking branch from 70c02e5 to e3cfaec Compare March 7, 2025 12:14
@emersion emersion assigned emersion and unassigned emersion Mar 7, 2025
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM ✅

Copy link
Contributor

@Synar Synar left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the component so I don't quite understand the use case, but the code LGTM

@emersion
Copy link
Member Author

emersion commented Mar 9, 2025

We've discussed out-of-band about the use-case, and Alice said this makes sense to her :)

@emersion emersion added this pull request to the merge queue Mar 9, 2025
Merged via the queue into dev with commit bb58acd Mar 9, 2025
6 checks passed
@emersion emersion deleted the emr/modular-picking branch March 9, 2025 11:29
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.

ui-charts: make picking layers generic
3 participants