-
Notifications
You must be signed in to change notification settings - Fork 35
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(page header): use picture tag - FRONT-3880 #2818
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.
Not related to this pull request, but i see in the usage page of the component that we always use and specify a 1/1 ratio for the thumbnail, but this was never enforced, apparently, i wonder whether it's the implementation that needs to be updated or the usage pages..
src/implementations/twig/components/page-header/page-header.html.twig
Outdated
Show resolved
Hide resolved
We will check with COMM for the image ratio |
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.
Why aren't we replacing also the background image in the page header with the picture @emeryro ..? It seems a much more relevant use case than the thumbnail itself for using it.
src/implementations/vanilla/components/page-header/page-header-print.scss
Show resolved
Hide resolved
@@ -169,7 +170,7 @@ | |||
} | |||
|
|||
.ecl-page-header--overlay-dark { | |||
.ecl-page-header__background::before { | |||
.ecl-page-header__background-container::before { | |||
background-color: rgba(map.get(theme.$color, 'black'), 0.5); |
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.
It seems we could set this overlay effect with a single css property, mix-blend-mode, https://developer.mozilla.org/en-US/docs/Web/CSS/mix-blend-mode, instead of using a pseudo element absolutely positioned, it's not supported in internet explorer, though, in case we are still bothered by that.
Also relevant for the banner pr, this property has been used for the "image-overlay" variant
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.
I tried it, but without success here.
As there is a blue background on the whole element, the overlay ended up being blue too. I'll double check
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.
@planctus I confirm that mix-blend-mode does not work as we would like to.
On the banner too, the result is not correct. It takes into consideration all the background set on the element, so the end result is something odd and depending on the background colors (this is clearly visible on the EU banner in your PR)
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.
ok, i will change this in the banner as well..
Replace thumbnail and background images tag by a picture