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

fix: ReactRouterOutlet interferes with layout #20771

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Conversation

Artur-
Copy link
Member

@Artur- Artur- commented Dec 20, 2024

Fixes #20767

Copy link

Test Results

1 159 files  ± 0  1 159 suites  ±0   1h 32m 34s ⏱️ -24s
7 579 tests ± 0  7 523 ✅ ± 0  56 💤 ±0  0 ❌ ±0 
7 926 runs   - 26  7 861 ✅  - 26  65 💤 ±0  0 ❌ ±0 

Results for commit 161179c. ± Comparison against base commit f081fc9.

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

@platosha does the change in #20770 with the new element fix this same issue as the created element sets display to contents?
The react-router-outlet is left without the style though so this might be needed.
I couldn't reproduce the issue with the grid not showing which might be because I don't have hilla 24.7.crudflow-SNAPSHOt available

@Artur-
Copy link
Member Author

Artur- commented Dec 21, 2024

The issue can be seen in https://github.com/Artur-/flow-crud-demo/tree/hilla, potentially requires removing some grid data binding not yet in Hilla snapshots. The crudflow snapshot is in the pre-releases repository so that one should work

@platosha
Copy link
Contributor

@platosha does the change in #20770 with the new element fix this same issue as the created element sets display to contents? The react-router-outlet is left without the style though so this might be needed. I couldn't reproduce the issue with the grid not showing which might be because I don't have hilla 24.7.crudflow-SNAPSHOt available

Separate issue. #20770 does not change the style of the ReactRouterOutlet wrapper, so this is still needed.

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Without display:contents the Grid component is not rendered at all, else rendered fine.

The only thing that I'm not entirely sure about is that VerticalLayout sets width:100% by default and this prevents react-router-outlet to show up always, because the side menu vertical layout takes all the width on the page. However, if I set it's width to auto, it shows up nicely.

That's probably fine as the example project uses divs and vertical layouts instead of AppLayout that has other width default apparently.

@mshabarov mshabarov merged commit bcd7a0d into main Jan 7, 2025
26 of 30 checks passed
@mshabarov mshabarov deleted the router-outlet-layout branch January 7, 2025 12:40
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.7.0.alpha3 and is also targeting the upcoming stable 24.7.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

react-router-outlet interferes with layouting / sizing of components
5 participants