-
Notifications
You must be signed in to change notification settings - Fork 13.5k
refactor(toolbar): improve slot layout and visibility handling #30371
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
base: next
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
3e47d64
to
5280f4f
Compare
8b66770
to
d593cb5
Compare
d593cb5
to
d9bcab6
Compare
4b8d8fa
to
f08f797
Compare
e94650e
to
9f9efb5
Compare
d277d5d
to
d3ac45d
Compare
d534843
to
132ae80
Compare
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.
A few minor changes requested. Looks really good!
Co-authored-by: Brandy Smith <[email protected]>
.../modal.e2e.ts-snapshots/modal-shape-sheet-default-ionic-md-ltr-light-Mobile-Chrome-linux.png
Outdated
Show resolved
Hide resolved
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.
…e up unnecessary space
…mework into next-branch/FW-6439
…mework into next-branch/FW-6439
…ent can be added dynamically
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.
This looks and works great! 🎉 Smart solution to a complex problem! In the future, we might want to explore relying more on Stencil for slot updates and width detection, but I don't think we need to hold this up. 👍
Issue number: resolves internal
What is the current behavior?
Currently, the Ionic theme toolbar uses
position: absolute
in modals to center its text or justtext-align: center
in other scenarios. This can lead to issues with alignment that can cause overlapping elements.What is the new behavior?
This new behavior reworks how slots get rendered and the content gets placed in the toolbar. It no longer relies on
host-context
at all (which doesn't work in Firefox) and instead usesflex
to lay out the toolbar and responsively react to the content around it.This does, however, have the drawback of preventing us from properly centering the content of the toolbar with just CSS, so there's new JavaScript that checks for content in certain slots and shows/hides slots based on whether or not there's anything in them, to allow the toolbar content to properly center itself.
toolbar-new.mp4
All of this combined leads to a much nicer toolbar experience that's actually responsive and works for any type of content - not just ion-titles.
Screenshots
Many of the screenshots had to be updated because the previous version of the toolbar was absolutely positioned in most cases and had a 3px top and bottom padding. This change removes that padding and makes it more closely align with the Figma mocks.
Does this introduce a breaking change?
Other information
Preview:
Toolbar