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(blockquote): use picture tag - FRONT-3873 #2784

Merged
merged 17 commits into from
Mar 8, 2023

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Feb 28, 2023

Use picture tag instead of background image

Notes:

  • data structure has been updated, but a backward compatibility is provided

@github-actions
Copy link

github-actions bot commented Feb 28, 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.

As in the inline comment, i would create a pr first for the introduction of the picture template and a separate one for the adoption in the blockquote

@planctus
Copy link
Contributor

planctus commented Mar 3, 2023

I will also add a test in the blockquote for the backward compatibility, i think this should always be the case

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.

Question:
I wanted to raise this in the picture pr, but then i convinced myself not to, now that i look at the blockquote it seems relevant again.
Why do we nest the img data in the picture object in the picture template?
If that is for the easyness of passing a single object, we have to consider that most of the replacements in the different components will need this deprecation while if we keep the img data separated from the picture data we are basically only extending the existing components by extending the needed data.

@emeryro
Copy link
Contributor Author

emeryro commented Mar 7, 2023

I find it cleaner that way: we pass the picture object, which contains everything needed to render the image. It also mimic the markup structure.
Currently there are not two component using the same data structure for images (sometimes it requires a 'path', sometimes a 'src', sometimes just a string, ...). So in any case we would have to update the structure of existing components, and having element in a dedicated structure would make it easier for the backward compatibility (we don't have to adapt it to every existing data structure)

@emeryro
Copy link
Contributor Author

emeryro commented Mar 8, 2023

We could flatten the structure in the picture component, having directly sources and image at root level, if that would make things better.

@planctus planctus removed the Question label Mar 8, 2023
planctus

This comment was marked as outdated.

@planctus planctus merged commit a7f5dd9 into v3.8.0-dev Mar 8, 2023
@planctus planctus deleted the FRONT-3873-picture-blockquote branch March 8, 2023 12:08
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