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-spacetimechart: use paths layer for conflicts #647

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

emersion
Copy link
Member

@emersion emersion commented Oct 15, 2024

The overlay layer is rendered on top of the captions, which is undesirable here.

Example bug:

out

@emersion emersion requested a review from a team as a code owner October 15, 2024 13:50
@emersion emersion force-pushed the emr/time-caption-above branch from 9e51598 to 9fdafcb Compare October 15, 2024 13:52
@emersion emersion self-assigned this Oct 15, 2024
Copy link
Contributor

@Akctarus Akctarus 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

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Doesn't seem like it fixes the issue ? Or did I do something wrong ?

Capture d’écran 2024-10-17 à 09 57 56

@clarani
Copy link
Contributor

clarani commented Oct 17, 2024

I still have the issue too :/

The overlay layer is rendered on top of the captions, which is
undesirable here.

Signed-off-by: Simon Ser <[email protected]>
@emersion
Copy link
Member Author

Apologies, I messed up when testing. I've updated the PR. Here's a hack patch to test this in the storybook:

diff --git a/ui-spacetimechart/src/stories/layers.stories.tsx b/ui-spacetimechart/src/stories/layers.stories.tsx
index 028ee5b5f118..2c6479c0f5d3 100644
--- a/ui-spacetimechart/src/stories/layers.stories.tsx
+++ b/ui-spacetimechart/src/stories/layers.stories.tsx
@@ -32,7 +32,7 @@ const CONFLICTS = [
     timeStart: +START_DATE + 35 * MINUTE,
     timeEnd: +START_DATE + 37 * MINUTE,
     spaceStart: 39 * KILOMETER,
-    spaceEnd: 41 * KILOMETER,
+    spaceEnd: 99999 * KILOMETER,
   },
 ];
 

@emersion emersion force-pushed the emr/time-caption-above branch from 9fdafcb to 7ffc56c Compare October 18, 2024 15:26
@emersion emersion changed the title ui-spacetimechart: draw TimeCaptions above other layers ui-spacetimechart: use paths layer for conflicts Oct 18, 2024
@emersion emersion requested a review from SharglutDev October 18, 2024 15:26
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm and tested. Should we add another commit to do the same for occupation blocks ?

@emersion
Copy link
Member Author

emersion commented Oct 21, 2024

That one uses the background layer, because it's drawn behind the paths (so it doesn't have the same bug).

@Akctarus Akctarus added this pull request to the merge queue Oct 21, 2024
Merged via the queue into dev with commit 87e47e8 Oct 21, 2024
6 checks passed
@Akctarus Akctarus deleted the emr/time-caption-above branch October 21, 2024 13:20
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.

4 participants