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.

Sign up for GitHub

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

Jump to bottom

ui-charts: replace several modules with ui-charts #931

Merged
merged 1 commit into from
Mar 5, 2025
Merged

ui-charts: replace several modules with ui-charts #931

merged 1 commit into from
Mar 5, 2025

Conversation

clarani
Copy link
Contributor

@clarani clarani commented Feb 24, 2025 •
edited
Loading

closes #929

This PR only moves code around and updates some imports.

@clarani clarani requested a review from a team as a code owner February 24, 2025 13:18
@clarani clarani marked this pull request as draft February 24, 2025 13:19
@clarani clarani force-pushed the cni/929-create-ui-charts branch 4 times, most recently from a5a3f55 to cf55c3e Compare February 25, 2025 15:15
@clarani clarani marked this pull request as ready for review February 25, 2025 15:16
@clarani clarani changed the title ui-charts: replace several modules by ui-charts ui-charts: replace several modules with ui-charts Feb 25, 2025
Yohh
Yohh reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@Yohh Yohh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

When merged, the ui-charts readme will be displayed at npm main page of @osrd-project/ui-charts, we should think about how we want to display charts readmes.

we should at least add a link to the existing ones

even if this readme is a good start, it should be enhanced with a chapter for each chart and at least a brief description.

Yohh
Yohh reviewed Feb 25, 2025
View reviewed changes
Yohh
Yohh reviewed Feb 25, 2025
View reviewed changes
@clarani clarani requested review from jacomyal and emersion February 25, 2025 15:54
jacomyal
jacomyal requested changes Feb 26, 2025
View reviewed changes
@Yohh Yohh self-assigned this Feb 26, 2025
@Math-R Math-R mentioned this pull request Feb 26, 2025
Merged
@clarani clarani force-pushed the cni/929-create-ui-charts branch from 5bd41e2 to b0aab57 Compare February 26, 2025 15:10
@jacomyal
Copy link
Contributor

LGTM, nice work 👍

Yohh
Yohh reviewed Feb 27, 2025
View reviewed changes
emersion
emersion reviewed Feb 28, 2025
View reviewed changes
@emersion
Copy link
Member

ui-manchette has been moved to manchetteWithSpaceTimeChart/. Should we move it to a manchette/ directory instead? That sounds like a better name to me.

Where should the useManchetteWithSpaceTimeChart hook and ManchetteWithSpaceTimeChart component end up going? In a subdir in manchette/, or in manchetteWithSpaceTimeChart/? Either option is fine by me.

@clarani clarani force-pushed the cni/929-create-ui-charts branch 3 times, most recently from baf1458 to e19d0b1 Compare March 4, 2025 15:33
@clarani
Copy link
Contributor Author

clarani commented Mar 4, 2025 •
edited
Loading

As discussed, I will re-organize the folders later 👍

@clarani clarani enabled auto-merge March 4, 2025 16:57
@clarani clarani force-pushed the cni/929-create-ui-charts branch from e19d0b1 to e738a14 Compare March 5, 2025 08:56
@clarani clarani requested a review from emersion March 5, 2025 09:00
emersion
emersion reviewed Mar 5, 2025
View reviewed changes
@clarani clarani force-pushed the cni/929-create-ui-charts branch from 33e1601 to 0aa81c0 Compare March 5, 2025 11:05
emersion
emersion reviewed Mar 5, 2025
View reviewed changes
emersion
emersion approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Nice, LGTM!

@clarani clarani force-pushed the cni/929-create-ui-charts branch from b75a3d8 to 2cb7f42 Compare March 5, 2025 11:16
@clarani clarani added this pull request to the merge queue Mar 5, 2025
Merged via the queue into dev with commit e40a486 Mar 5, 2025
6 checks passed
@clarani clarani deleted the cni/929-create-ui-charts branch March 5, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacomyal jacomyal jacomyal approved these changes

@emersion emersion emersion approved these changes

@Yohh Yohh Yohh approved these changes

Assignees

@Yohh Yohh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

create ui-charts
4 participants
@clarani @jacomyal @emersion @Yohh