-
Notifications
You must be signed in to change notification settings - Fork 13
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
refactor(sbb-dialog): introduce inner components and header animation #2231
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2231 +/- ##
=======================================
Coverage ? 93.18%
=======================================
Files ? 309
Lines ? 25307
Branches ? 2056
=======================================
Hits ? 23582
Misses ? 1693
Partials ? 32 ☔ View full report in Codecov by Sentry. |
804100f
to
8ae6662
Compare
8ae6662
to
f8814bf
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.
Thanks for addressing this!
Code looks ok so far, but testing it in real life, the initial scrolling looks buggy for me as it forces the user to scroll down at least the height of the header. Is this really intended?
Please also wait for approval of @mcilurzo
@jeripeierSBB Yes, I know what you mean, but I have already shown it to Manuel and it is fine with him. But if you want we can show it to him again and discuss it, although I can't think of any other possible alternatives at the moment. |
f8814bf
to
1242542
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.
From coding perspective it looks good for me :). Not sure, but if Manuel really is aware of, maybe we can accept it like it is. Maybe a second opinion of @kyubisation?
0700430
to
bac5d1c
Compare
9a40e4b
to
c6984bd
Compare
We would like to discuss with @mcilurzo if there is the need of a property to opt out of the header hiding feature. potentially providing a breakpoint range from where it stops or starts... to be discussed. |
fae5632
to
f30bb00
Compare
15fea8d
to
e37dea0
Compare
63a9d68
to
bff75f2
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.
LGTM, thanks!!
BEGIN_COMMIT_OVERRIDE
fix(sbb-dialog): fix accessibility with option to hide the header on scroll
BREAKING CHANGE: The
sbb-dialog
component now needs the dedicated inner elementssbb-dialog-title
,sbb-dialog-content
, andsbb-dialog-actions
. Use these components to respectively provide a title, a content and, optionally, a footer with an action group. Moreover, the full-screen variant (which occurred when no title was provided to the dialog) has been removed. To achieve a full-screen overlay, please use the newsbb-overlay
component.As a migration help, consider the following example. Old:
<sbb-dialog title-content="Title"><p>Dialog content.</p><sbb-action-group slot="action-group">...</sbb-action-group></sbb-dialog>
.New:
<sbb-dialog><sbb-dialog-title>Title</sbb-dialog-title><sbb-dialog-content><p>Dialog content</p></sbb-dialog-content><sbb-dialog-actions>...</sbb-dialog-actions></sbb-dialog>
.Previously, a full-screen dialog was displayed if no title was provided to the dialog component:
<sbb-dialog><p>Dialog content.</p></sbb-dialog>
. To achieve the same, it is now mandatory to use thesbb-overlay
component:<sbb-overlay><p>Overlay content.</p></sbb-overlay>
.END_COMMIT_OVERRIDE
Closes #2019
BREAKING CHANGE: The
sbb-dialog
component now supports the dedicated inner elementssbb-dialog-title
,sbb-dialog-content
, andsbb-dialog-actions
. Use these components to respectively provide a title, a content and, optionally, a footer with an action group. Moreover, the full-screen variant (which occurred when no title was provided to the dialog) has been removed. To achieve a full-screen overlay, please use thesbb-overlay
component.This was the previous implementation:
This is the new implementation:
Previously, a full-screen dialog was displayed if no title was provided to the dialog component:
It is now possible to use an
sbb-overlay
component to display the same variant: