Skip to content

Conversation

@mattkubej
Copy link
Contributor

@mattkubej mattkubej commented May 23, 2023

WHY are these changes introduced?

Fixes #9252

Prevents Form from overriding parent border radius by inheriting the parent border radius.

WHAT is this pull request doing?

Adds an inline style to the Form component for inheriting border radius.

How to 🎩

Navigate to Inventory Index and validate the bottom corners of the Inventory Index Table are rounded.

Codesandbox with fix

🎩 checklist

@mattkubej mattkubej force-pushed the form/border-radius-inherit branch from 262159a to d0240a9 Compare May 23, 2023 14:53
@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2023

size-limit report 📦

Path Size
polaris-react-cjs 243.05 KB (+0.01% 🔺)
polaris-react-esm 157.33 KB (-0.01% 🔽)
polaris-react-esnext 219.86 KB (+0.03% 🔺)
polaris-react-css 47.45 KB (+0.02% 🔺)

@mattkubej
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

🫰✨ Thanks @mattkubej! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230523145723
yarn add @shopify/polaris@0.0.0-snapshot-release-20230523145723

@mattkubej mattkubej force-pushed the form/border-radius-inherit branch from 39a737e to fc2c64a Compare May 23, 2023 17:26
@mattkubej mattkubej force-pushed the form/border-radius-inherit branch from f887ab7 to 55a23ab Compare May 23, 2023 17:39
@mattkubej
Copy link
Contributor Author

/snapit

@github-actions
Copy link
Contributor

🫰✨ Thanks @mattkubej! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230523175205
yarn add @shopify/polaris@0.0.0-snapshot-release-20230523175205

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Approving as I can see it fixes the issue. I don't want to block a fix going out.

However this PR is very oddly specific to the IndexTable -> Form relationship. Could we resolve this in the IndexTable component? I feel like a developer working on Form in the future will be unsure why this was added as it's not clear what it resolved. It could then be removed and cause this issue again.

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Approving as I can see it fixes the issue. I don't want to block a fix going out.

However this PR is very oddly specific to the IndexTable -> Form relationship. Could we resolve this in the IndexTable component? I feel like a developer working on Form in the future will be unsure why this was added as it's not clear what it resolved. It could then be removed and cause this issue again.

@mattkubej
Copy link
Contributor Author

mattkubej commented May 24, 2023

However this PR is very oddly specific to the IndexTable -> Form relationship. Could we resolve this in the IndexTable component? I feel like a developer working on Form in the future will be unsure why this was added as it's not clear what it resolved. It could then be removed and cause this issue again.

I do not think this is necessarily specific to the relationship between IndexTable and Form, but rather when we have any child element within a LegacyCard that does not inherit the border-radius of the LegacyCard and sits at the corner of the LegacyCard. This will result in the element overflowing the radius and utilizing whatever border it has set (or the absence of one).

It does seem like there is a reliance on chaining border-radius inherit to insure that the LegacyCard's border radius does appear when an element sits on the corner, which is fairly brittle and why we're seeing it in the situation highlighted in this ticket. The Form element breaks the chain of passing the inherited border-radius of the LegacyCard.

An alternative to this chaining of border-radius inherit styles through the children of the LegacyCard could be setting overflow hidden on the LegacyCard, so it provides corner clipping. This might be a more appropriate solution than what I have within this PR, but the change could be more impactful than what I have here. So, we'd want to do a reasonable sanity check across Admin.

I'll hold off on delivering anything yet, because I do not think the change in this PR is urgent and would rather solve this in the most reasonable way up front.

@alex-page
Copy link
Member

alex-page commented May 24, 2023

Yes. Thank you @mattkubej for understanding and diving into what I wrote and identifying it as children overflowing parent border radius on legacy card + card.

I would love to see a snap it in web of adding the overflow hidden on Card and see how much breaks. I feel conflicted on what the best solution here is but I also find it weird that we wouldn't have overflow hidden by default when a parent has a border radius.

@mattkubej
Copy link
Contributor Author

mattkubej commented May 24, 2023

I would love to see a snap it in web of adding the overflow hidden on Card and see how much breaks. I feel conflicted on what the best solution here is but I also find it weird that we wouldn't have overflow hidden by default when a parent has a border radius.

I can get that drafted up in a spin instance and codesandbox tomorrow.

I think I'd prefer the overflow hidden solution as I feel the chaining is more brittle. I'd imagine it's likely fairly safe, because I'd assume most of our cases of LegacyCard do not have a child element hugging a corner due to the padding provided by LegacyCard.Section.

@mattkubej
Copy link
Contributor Author

I am going to close this pull request for now, because I think overflow hidden is likely the way to go here.

I created the following PR, which has a storybook and web spin instance linked. From looking at Chromatic, it appears that it might have addressed some other minor issues, which were difficult to see, such as a border sitting on top of the border radius.

@mattkubej mattkubej closed this May 24, 2023
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.

[Form] Does not inherit border radius, removes border radius from parent Card

2 participants