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

[SFS-1685] Feature/height siteeditor #96

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

lucvysk
Copy link
Contributor

@lucvysk lucvysk commented Oct 16, 2024

What problem is this solving?

Hi, this PR tries to solve these two:
#87
#90

How to test it?

https://vysk2--hunterdouglasnl.myvtex.com/
https://vysk--puntoscolombia.myvtex.com/

Explaining

imageDimensions is used inline in the tag <img>. Therefore it overwrites any css without important. That breaks many stores using dynamic dimensions.

I was thinking in remove entirely the inline style={}, but since it's a commit from 5yo I think should be better to
be planned/tested. Then, the height is removed from the inline rule and added just when the component have experimentalSetExplicitDimensions as true.

@lucvysk lucvysk requested review from a team as code owners October 16, 2024 16:32
@lucvysk lucvysk requested review from vsseixaso, RodrigoTadeuF and leo-prange-vtex and removed request for a team October 16, 2024 16:32
Copy link

vtex-io-ci-cd bot commented Oct 16, 2024

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch (backwards-compatible bug fixes)

  • Minor (backwards-compatible functionality)

  • Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

Copy link

github-actions bot commented Jan 29, 2025

Warnings
⚠️

Changes were made to package.json, but not to yarn.lock - Perhaps you need to run yarn install?

Generated by 🚫 dangerJS against 487441a

react/package.json Dismissed Show dismissed Hide dismissed
react/package.json Dismissed Show dismissed Hide dismissed
react/package.json Dismissed Show dismissed Hide dismissed
react/package.json Dismissed Show dismissed Hide dismissed
react/package.json Dismissed Show dismissed Hide dismissed
react/package.json Dismissed Show dismissed Hide dismissed
@iago1501 iago1501 requested a review from a team as a code owner January 29, 2025 19:02
Copy link
Contributor

@iago1501 iago1501 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a slightly different approach to the solution and committed directly to the branch.

To prevent dimensions from being added incorrectly to stores, we'll have some conditionals that need to be met for them to be applied. You can test it here: https://iespinozaimage--koestore.myvtex.com/?v=12313

  • The dimensions must be explicitly added to the store; they cannot be added with percentages (%). If any dimension has a percentage, the code will not apply the dimensions to the HTML.

  • I maintained the default behavior of adding dimensions to CSS inline if they are not explicitly applied.

  • If dimensions are explicitly added, the CSS inline will only define the maximum size of the dimensions.

  • I added a new field called specificHeight, which is a height specifically defined within the image, without initially assigning it to the value of height, as we don't know the implications of doing so. I maintained the original field definition inherited from the parent (list-context).

  • The priority order for height is: specificHeight (image component height), genericHeight (value of height that can be sent if not set by specificHeight; this was the default previously, so if no specificHeight is set, it will remain), and maxHeight (parent height, list-context).

  • Due to the possibility of empty values being allowed, some conditionals for falsy checks were added.

@iago1501 iago1501 requested review from vmourac-vtex and removed request for RodrigoTadeuF and leo-prange-vtex January 31, 2025 18:08
@iago1501 iago1501 added the enhancement New feature or request label Jan 31, 2025
@iago1501 iago1501 changed the title Feature/height siteeditor [SFS-1685] Feature/height siteeditor Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants