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: fix blank stories #858

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Conversation

emersion
Copy link
Member

@emersion emersion commented Jan 27, 2025

The space time chart stories were sometimes not loading properly, showing a blank page.

Two issues:

  • All stories were passing className='absolute inset-0' to SpaceTimeChart. However, SpaceTimeChart was combining these classes with 'relative'. As a result, the browser would roll a dice and pick either 'absolute' or 'relative'. When 'relative' is picked first, 'inset-0' has no effect because…
  • All stories except one were using a wrapper div with className="inset-0". This had no effect because this div is neither absolute nor relative.

As a result, half the time the chart would have a zero height.

Fix this by:

  • Adding absolute positioning to the wrapper div.
  • Setting the height of the SpaceTimeChart instead of mixing up conflicting positioning classes.

@emersion emersion requested review from Synar and clarani January 27, 2025 10:17
@emersion emersion requested a review from a team as a code owner January 27, 2025 10:17
@emersion emersion force-pushed the emr/spacetimechart-sizing branch from 40c8160 to 9aff6b7 Compare January 27, 2025 10:17
The space time chart stories were sometimes not loading properly,
showing a blank page.

Two issues:

- All stories were passing className='absolute inset-0' to SpaceTimeChart.
  However, SpaceTimeChart was combining these classes with 'relative'.
  As a result, the browser would roll a dice and pick either 'absolute'
  or 'relative'. When 'relative' is picked first, 'inset-0' has no
  effect because…
- All stories except one were using a wrapper div with
  className="inset-0". This had no effect because this div is neither
  absolute nor relative.

As a result, half the time the chart would have a zero height.

Fix this by:

- Adding absolute positioning to the wrapper div.
- Setting the height of the SpaceTimeChart instead of mixing up
  conflicting positioning classes.

Signed-off-by: Simon Ser <[email protected]>
@emersion emersion force-pushed the emr/spacetimechart-sizing branch from 9aff6b7 to a612c9d Compare January 27, 2025 10:21
@emersion emersion self-assigned this Jan 27, 2025
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.

Overall LGTM. This bug was extremely annoying when working on the component, so thanks a lot!

Tangent to this PR, but in the osrd repo we also use <className="inset-0 absolute h-full">, should we remove the absolute and inset-0?
More generally there seem to be a lot of conflicting positioning class uses in this component and its wrappers, which could use a cleanup.

@emersion
Copy link
Member Author

Tangent to this PR, but in the osrd repo we also use <className="inset-0 absolute h-full">, should we remove the absolute and inset-0?

Yes, I think so. We specify a fixed height attribute so all of these classes can be dropped.

Copy link
Contributor

@anisometropie anisometropie left a comment

Choose a reason for hiding this comment

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

tested, thanks!

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 ✅

@emersion emersion enabled auto-merge February 4, 2025 18:21
@emersion emersion added this pull request to the merge queue Feb 4, 2025
Merged via the queue into dev with commit 54e521d Feb 4, 2025
6 checks passed
@emersion emersion deleted the emr/spacetimechart-sizing branch February 4, 2025 18:24
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