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-speedspacechart: fix speedlimittags layer blinking #633

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

Synar
Copy link
Contributor

@Synar Synar commented Oct 10, 2024

Close #427

There were 2 issues I could find leading to the blinking :

  • the store object was used as a dependency of useEffect, which means it was triggered every time
  • the tooltip computation and the canvas drawing were tied together in a single function, despite the tooltip computation depending on the cursor position (and we don't want the canvas to be redrawn whenever the cursor moves)

There seems to be an overreliance on the store object in the code, but it may take a major refactor to fix it.

@Synar Synar requested a review from a team as a code owner October 10, 2024 17:02
@Synar
Copy link
Contributor Author

Synar commented Oct 10, 2024

Close #427 on osrd-ui

Therefore fix half of OpenRailAssociation/osrd#8853 on osrd

@Synar Synar force-pushed the ali/fix-blinking-ui-speedspacechart branch from ee55088 to eb498bf Compare October 10, 2024 17:13
@emersion emersion self-requested a review October 10, 2024 17:21
Copy link
Contributor

@Yohh Yohh left a comment

Choose a reason for hiding this comment

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

very nice!

a few comments before reviewing deeper

@Yohh
Copy link
Contributor

Yohh commented Oct 10, 2024

I can see a little unwanted space at the end of some rects, depending on the zoom level:
image

@Yohh
Copy link
Contributor

Yohh commented Oct 10, 2024

I take the liberty of assigning @Caracol3 , he has a pending PR that will have many conflicts after this one

@Yohh Yohh requested a review from Caracol3 October 10, 2024 21:08
@Synar
Copy link
Contributor Author

Synar commented Oct 11, 2024

I can see a little unwanted space at the end of some rects, depending on the zoom level:
image

Is this behavior that wasn't present before the pr? I tried not to touch the drawing code and dimension computations at all, so if something changed there I messed up

@Yohh
Copy link
Contributor

Yohh commented Oct 11, 2024

I can see a little unwanted space at the end of some rects, depending on the zoom level:

Is this behavior that wasn't present before the pr? I tried not to touch the drawing code and dimension computations at all, so if something changed there I messed up

you're right, it's not your work, that was already present

@emersion
Copy link
Member

There were 2 issues I could find leading to the blinking

Next time, if you're feeling up for it, would be easier for reviewers if these two changes were split into separate commits!

@Synar
Copy link
Contributor Author

Synar commented Oct 11, 2024

I take the liberty of assigning @Caracol3 , he has a pending PR that will have many conflicts after this one

Thanks! I'll confess I did not notice there was another PR on this component. But I don't mind being the one to do the conflict resolution

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.

I'm mostly nitpicking at this point. Feel free to discard/ignore comments you disagree with.

@Synar Synar force-pushed the ali/fix-blinking-ui-speedspacechart branch from d795829 to ee0a7aa Compare October 23, 2024 10:17
@Synar Synar enabled auto-merge October 23, 2024 10:23
@Synar Synar requested review from emersion and removed request for clarani October 23, 2024 10:24
@Synar Synar added this pull request to the merge queue Oct 23, 2024
Merged via the queue into dev with commit d6fc27d Oct 23, 2024
6 checks passed
@Synar Synar deleted the ali/fix-blinking-ui-speedspacechart branch October 23, 2024 13:44
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.

Speed space chart is blinking
3 participants