-
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
fix(sbb-image): fix alt attribute and provide css var for aspect-ratio #2607
fix(sbb-image): fix alt attribute and provide css var for aspect-ratio #2607
Conversation
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 your contribution. I think, as mentioned, we should rename the variable with a breaking change.
src/components/image/image.ts
Outdated
@@ -275,6 +275,12 @@ export class SbbImageElement extends LitElement { | |||
* Set an aspect ratio | |||
* default is '16-9' (16/9) | |||
* other values: 'free', '1-1', '1-2', '2-1', '2-3', '3-2', '3-4', '4-3', '4-5', '5-4', '9-16' | |||
* If the aspectRatio value is set to 'free', the component sets the CSS declaration to |
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.
As we soon will release version 1.0, I think we should "sbb-"-prefix the very last not prefixed CSS var in the project :) this is causing a breaking change but it is ok for now. Furthermore, you could add a specific entry in the jsdoc section of the SbbImageElement class with "@cssprop":
@cssprop [--sbb-image-aspect-ratio=auto] - TBD...
With that it appears in readme.md and is therefore well documented and part of the public API.
see e.g. logo component for an example
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 hint, I have indeed overlooked the documentation by @cssprop. It's added now.
I also renamed the variable. Since it is freshly introduced, it should not cause any breaking change? Or are you referring to --image-border-radius?
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.
yes, I just realized it during re-review :D Probably I will create a follow-up PR for that. BTW, the image is, besides some minimal refactorings, still in the state you guys implemented it :)
Sure thing, thanks for the quick response! Please see the separate comment regarding the renaming/breaking change. |
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, thank you
Preflight Checklist
Issue
There's no connected lyne-components issue.
Pull request checklist
Changes
Changes in this pull request:
alt
attribute is always present onimg
elements, even if there is no alt text. Otherwise, supporting technologies may be tempted to interpret the image.aspectRatio
value is set tofree
, the component sets the CSS declaration toaspect-ratio: var(--image-aspect-ratio, auto);
instead ofaspect-ratio: auto;
. This would allow consumers to enter the value for the CSS variable--image-aspect-ratio
from outside the component. This would make it possible to set a different aspect ratio from viewport to viewport. People tried to achieve that behaviour with a workaround by defining two separate image components as can be seen here: https://www.sbbinsurance.li/ (inspect the markup of the hero image). This little fix would allow a lot of additional flexibility without having to break any existing functionality.Browsers
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?