Skip to content
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

Merged
merged 10 commits into from
Apr 3, 2023

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Mar 30, 2023

Replace thumbnail and background images tag by a picture

@github-actions
Copy link

github-actions bot commented Mar 30, 2023

Copy link
Contributor

@planctus planctus left a 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..

@emeryro
Copy link
Contributor Author

emeryro commented Mar 30, 2023

We will check with COMM for the image ratio

Copy link
Contributor

@planctus planctus left a 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.

@planctus planctus removed the Question label Apr 3, 2023
@@ -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);
Copy link
Contributor

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

Copy link
Contributor Author

@emeryro emeryro Apr 3, 2023

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

Copy link
Contributor Author

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)

Copy link
Contributor

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..

@planctus planctus merged commit 0e9db9a into v3.8.0-dev Apr 3, 2023
@planctus planctus deleted the FRONT-3880-page-header-picture branch April 3, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants