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

fix: fixed issue with border for image #3548

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

neatbyte-vnobis
Copy link
Collaborator

@neatbyte-vnobis neatbyte-vnobis commented Sep 22, 2023

Changes

Resolve issue "Image border not applied correctly"
Task provided by @SvenAlHamad no reference to issue or Notion.

Before Changes
Знімок екрана 2023-09-22 о 16 11 32

After Changes
Знімок екрана 2023-09-22 о 16 09 56

How Has This Been Tested?

Manually

Documentation

None

@neatbyte-ivelychko
Copy link

There are two solutions for this issue.
First of the solutions is used in this PR, and the second one I will provide in this comment.

In the second solution we would not need to use useEffect and loop through the styles for the element as we do in the first solution. What we need is to use

const borderStyles = border({ element: { ...element, type: "" }, theme });
const borderAssignedStyles = assignStyles({
   breakpoints: theme.breakpoints,
    styles: borderStyles || {}
});
const PbImg = styled.img({
  width: element.data.image.width,
  height: element.data.image.height,
  maxWidth: "100%",
  ...borderAssignedStyles
});

in packages/app-page-builder-elements/src/renderers/image.tsx.

And we should add additional if statement here packages/app-page-builder-elements/src/modifiers/styles/border.ts

if (element.type === "image") {
        return {};
}

And it would also work.
@adrians5j What do you think?

@neatbyte-vnobis
Copy link
Collaborator Author

/cypress

@github-actions
Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

@adrians5j
Copy link
Member

Will need to think a bit about this. Will need to see if we can do this in a better way.

@adrians5j adrians5j self-assigned this Oct 13, 2023
@neatbyte-vnobis
Copy link
Collaborator Author

@adrians5j Any new thoughts on this PR? Or should we stick with one of the provided solutions?

@neatbyte-vnobis
Copy link
Collaborator Author

/cypress

Copy link

github-actions bot commented Dec 5, 2023

Cypress E2E tests have been initiated (for more information, click here). ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants