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

manchette with spacetimechart, hook integration #504

Merged
merged 2 commits into from
Sep 26, 2024
Merged

manchette with spacetimechart, hook integration #504

merged 2 commits into from
Sep 26, 2024

Conversation

Math-R
Copy link
Contributor

@Math-R Math-R commented Sep 18, 2024

No description provided.

@Math-R Math-R changed the title wip manchette with spacetimechart, hook integration Sep 20, 2024
@SharglutDev SharglutDev mentioned this pull request Sep 20, 2024
Merged
@Math-R Math-R force-pushed the mrd/get-manchette-module branch from bab4831 to 3d2dcd3 Compare September 24, 2024 04:32
@Math-R Math-R marked this pull request as ready for review September 24, 2024 04:35
@Math-R Math-R requested a review from a team as a code owner September 24, 2024 04:35
@Math-R Math-R force-pushed the mrd/get-manchette-module branch from 3d2dcd3 to 3cc52dd Compare September 24, 2024 04:39
clarani
clarani reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@clarani clarani 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.

Thanks for the PR, good work !

I found some small bugs:

  • ✅ when I zoom in km mode, the scale of the GET is not correct anymore My bad, I checked and it's the data which is not correct
    image
  • I can't zoom in the time anymore (maj+scroll)

clarani
clarani reviewed Sep 24, 2024
View reviewed changes
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.

I added some comments.

  • You have some warnings in the console
  • Don't forget to clean the ui-manchette folder

kmer2016
kmer2016 reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@kmer2016 kmer2016 left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! I have a few suggestions, but the component works well, and I didn't find any bugs in the new component. However, the stories for OperationalPoint, OperationalPointList, and Manchette are empty. Should we consider deleting them?

Also, there are some warnings from the linter.

@Math-R Math-R force-pushed the mrd/get-manchette-module branch 2 times, most recently from 38aebec to ae6d2c7 Compare September 25, 2024 14:56
@Math-R
Copy link
Contributor Author

Math-R commented Sep 25, 2024

Thanks for the PR, good work !

I found some small bugs:

  • ✅ when I zoom in km mode, the scale of the GET is not correct anymore My bad, I checked and it's the data which is not correct
    image
  • I can't zoom in the time anymore (maj+scroll)

The zoom x is now fixed

clarani
clarani reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@clarani clarani 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.

In ui-manchette:

  • you can also remove the files ./consts.ts
  • you need to update the sampleData too (clean SAMPLE_PATH_PROPERTIES_DATA and remove SAMPLE_PATHS_DATA)
  • in the story, remove the const PathData and directly give the SAMPLE_PATH_PROPERTIES_DATA.operational_points line 34 in the args
  • maybe remove the css of the manchette
  • can you check that the utils are still necessary ?

@Math-R
Copy link
Contributor Author

Math-R commented Sep 25, 2024 •
edited
Loading

$

In ui-manchette:

  • you can also remove the files ./consts.ts

not entirely

  • you need to update the sampleData too (clean SAMPLE_PATH_PROPERTIES_DATA and remove SAMPLE_PATHS_DATA)
  • in the story, remove the const PathData and directly give the SAMPLE_PATH_PROPERTIES_DATA.operational_points line 34 in the args

I think the stories need a rework if we would like to keep it

  • maybe remove the css of the manchette

Can't remove the css because it is useful here, manchette-with-spacetimechart just provide a hook and an example of integration but the Manchette the OperationalPointList and operationalPoint still need their css

  • can you check that the utils are still necessary ?

I clean the file but there is still one util used

@Math-R Math-R requested review from jacomyal and clarani September 25, 2024 17:04
kmer2016
kmer2016 approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@kmer2016 kmer2016 left a comment

Choose a reason for hiding this comment

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

Good job ! LGTM

@jacomyal
Copy link
Contributor

LGTM 👍

clarani
clarani reviewed Sep 26, 2024
View reviewed changes
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.

Almost good, there is still some warnings in the console:
image

@Math-R Math-R force-pushed the mrd/get-manchette-module branch from d4c618b to d17207a Compare September 26, 2024 08:52
@Math-R
Copy link
Contributor Author

Math-R commented Sep 26, 2024

Almost good, there is still some warnings in the console: image

it's fixed !

@Math-R Math-R force-pushed the mrd/get-manchette-module branch 3 times, most recently from bf42842 to 74914bb Compare September 26, 2024 09:37
@Math-R
ui-manchette-with-spactimechart: new implementation of manchette with…
ecc55cc 
… spacetimechart, add some unit tests

- remove spacetimechart dependency from manchette
- Move styles from manchette to manchette-with-spacetimechart, and change the html layout
- implement a custom hook usemanchettewithspacetimechart to hydrate manchette and spacetimechart props

Signed-off-by: Mathieu Richard <[email protected]>
@Math-R Math-R force-pushed the mrd/get-manchette-module branch from 74914bb to e1d3bd8 Compare September 26, 2024 09:39
@Math-R Math-R force-pushed the mrd/get-manchette-module branch from e1d3bd8 to 9b38541 Compare September 26, 2024 09:49
@Math-R Math-R added this pull request to the merge queue Sep 26, 2024
Merged via the queue into dev with commit 4cfb8a7 Sep 26, 2024
6 checks passed
@Math-R Math-R deleted the mrd/get-manchette-module branch September 26, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clarani clarani clarani left review comments

@jacomyal jacomyal jacomyal approved these changes

@kmer2016 kmer2016 kmer2016 approved these changes

@Yohh Yohh Yohh approved these changes

@anisometropie anisometropie anisometropie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Math-R @jacomyal @kmer2016 @Yohh @anisometropie @clarani