-
-
Notifications
You must be signed in to change notification settings - Fork 902
Avoid expansion-item stutters by respecting Quasar expectations during animation #5659
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
Avoid expansion-item stutters by respecting Quasar expectations during animation #5659
Conversation
falkoschindler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great approach, @evnchn! It seems to work flawlessly in many cases.
But setting display: block changes the layout when the content relies on flex layout:
with ui.expansion('Expansion'):
with ui.row().classes('border'):
ui.label('Hello')
ui.label('world')The row resizes during animation, which it didn't before this PR. Maybe we can explicitly set gap: 0 instead of changing display?
Fixes your example but breaks the main one in the PR description above: with ui.expansion('Expansion'):
with ui.item():
with ui.item_section():
ui.item_label('Line 1')
ui.item_label('Line 2')I think it's just trial and error and see what works... |
|
Opus 4.5 suggests to add During animation, However, switching to block layout changes how children are sized:
This causes elements like Adding |
|
But with ui.expansion('Expansion').classes('w-100'):
ui.label('Hello ' * 100).classes('border')😕 |
|
I found a solution that seems to work: Introduce an inner wrapper div that hosts the flex layout, while leaving the outer New file: export default {
template: `
<q-expansion-item ref="qRef">
<div class="nicegui-expansion-content">
<slot></slot>
</div>
</q-expansion-item>
`,
};
class Expansion(..., component='expansion.js', ...):
Benefits
|
|
Thank god we have pytests! This PR breaks the ability to define custom header slots: with ui.expansion() as expansion:
with expansion.add_slot('header'):
ui.label('Header') # not displayed
ui.label('Content') |
falkoschindler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have a final solution. 🤞🏻
@evnchn Do you agree?
|
Tried everything above and it seems to work. I don't really have a systematic way of discovering further edge cases though. At least now that we have bdb28f0 out of the way, I believe that every content should show up even if the stuttering persists, so the impact is manageable. |
Motivation
In #4918 we see that Quasar expansion "jumps" when opening and closing.
Analysis
Previous analysis by @platinops points out how
height: 80pxis being set in the intermediate transition process.Here comes the eureka moment: during expansion opening and closing, Quasar evaluates the height of the children, and animates appropriately.
Implementation
The following has been found to be harmful to the heigh evaluation process:
padding: 0 var(--nicegui-default-padding);content: ""in::beforeand::after(new in this PR): hence why it is removed.But we still need the padding, so what do we do?
margin: var(--nicegui-default-padding) 0;The plain
ui.expansioncase still breaks:display: block;is found to workBut we want to minimize the intervention:
heightis explcitly set by Quasar:[style*="height"]Progress
Test script shortcuts
Simplified version: refer to #4918 (comment)
Comprehensive version: refer to #4918
Both work flawlessly with this PR
Final notes
CSS changes are easy to make mistake. I have make mistakes before, and I would like more eyes on this to ensure we did not regress anywhere.