-
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
feat(sbb-teaser): redesign #2211
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2211 +/- ##
=======================================
Coverage ? 93.59%
=======================================
Files ? 222
Lines ? 23278
Branches ? 2081
=======================================
Hits ? 21786
Misses ? 1455
Partials ? 37 ☔ View full report in Codecov by Sentry. |
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.
Good job 👍
Some fixes and some discussion topic
d715726
to
0cfa3da
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.
Last review and then we can merge it I guess 😸
Also, we need to rebase from master and rename SbbTeaser
into SbbTeaserElement
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 the great rework!
Additionally to the comments I have two questions (probably for @mcilurzo )
- What should be the behavior when there is more text than the image? Should that be defined somehow? e.g. when end-centered should the image anyways be at the top? or should the centering win and grow over the image?
- I think, the hover state here does not look correctly?
46a9823
to
bd9a8cd
Compare
e861896
to
2a50c71
Compare
I checked the teaser and it looks almost perfect. I only found 2 small issues: |
We need to wait with merge a little while as it is a breaking change and we are still waiting until consumers have been migrated to lit. |
BREAKING CHANGE: The property |
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, wait with merge
Quick question; Is it necessary to prefix the component name in the breaking changes? My assumption would be, that since the component name is part of the PR title, release-please would extract it from there. |
Not 100% sure but we can try |
6179f58
to
2f1a8ad
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.
Generally LGTM
Some concerns
BREAKING CHANGE: The property
isStacked
has been removed in favor ofalignment
. Please see the documentation for further info. Thedescription
is not clamped to two lines anymore (responsibility of consumer). The slottedimage
has now a default width of300px
. The slot, formerly nameddescription
, has been replaced by the unnamed slot. Support of nestedp
elements dropped (invalid html).Closes #1897
Preflight Checklist
Issue
This PR Closes #1897
Pull request checklist
Please check if your PR fulfills the following requirements:
See Review Guidelines for more information what is checked during review process.
Changes
Changes in this pull request:
Browsers
I tested the build on the following browsers:
Screen readers
I tested the build on the following browsers:
Pull request type
Please check the type of change your PR introduces:
Does this introduce a breaking change?
Other information