Replies: 1 comment
-
Certainly agree that there are many cases where margin (particularly negative margin) is causing friction. I think this is worth exploring. I could see it removing the need for a lot of hacky overrides for component compositions. There are several areas where we use it internally within components (ex: DatePicker) that might be exceptions. However, all components where children render within the component (ex: IndexTable) or has margin on the outside of the component (ex: Button, Icon) seem like valid candidates for removing margin. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Intro
Polaris provides a great set of lego blocks to build out apps in Shopify admin. These lego blocks, mainly, fit nicely together and provide a cohesive component library and allow for fitting these pieces together to form a larger vision of design, features, and products. Margins, all too often, can turn this lego build into a jenga tower.
I think Theo does a good job explaining why margin might be harmful, and part of it comes from a blog post from Max Stoiber called "Margin considered harmful". In essence: "Padding is taking distance from someone, margin is pushing someone. Pushing someone is bad"
Issues
Negative margins on buttons and icons cause unwanted scrollbars (both vertically and horizontally) on mobile
![image](https://private-user-images.githubusercontent.com/1019769/308665184-768270d3-4dcd-411b-ab4a-992abe6b1636.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg5MjY0MzIsIm5iZiI6MTczODkyNjEzMiwicGF0aCI6Ii8xMDE5NzY5LzMwODY2NTE4NC03NjgyNzBkMy00ZGNkLTQxMWItYWI0YS05OTJhYmU2YjE2MzYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDdUMTEwMjEyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ODA4NGViMTI5MTRkMTU0MTJjMjFkODhlMmNjYWMxZjQzN2Q1ZWQ1YjY1NjI4MjIzNjQ3NGY4ZWYwYjE1YjFiNiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.e6AOgiI15d-1gJ8U_v6ocMCuU5Vb9DGGxn1HJbeNRlw)
Negative margins cause vertical misalignment of buttons in IndexTable rows
![image](https://private-user-images.githubusercontent.com/1019769/308665285-92476949-37fd-41cd-afbd-8bd1792c32eb.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg5MjY0MzIsIm5iZiI6MTczODkyNjEzMiwicGF0aCI6Ii8xMDE5NzY5LzMwODY2NTI4NS05MjQ3Njk0OS0zN2ZkLTQxY2QtYWZiZC04YmQxNzkyYzMyZWIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwNyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDdUMTEwMjEyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MjIwYTllOWU0MGI4MTE5ZDRkODcyMjMxNTU5ZjY4NmJjZDA3ZGNlNWYxOTcyMmIxNDFhNjRmYWViZmU4MjllMCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.SnrzTlR2UKF5WhnoumgE-9jO0GDPxVdzvc9iSsblZzI)
Automatic margins on banners causes misalignment, even inside of BlockStack
https://github.com/Shopify/discovery-app/pull/4455#issuecomment-1969447293
Although the admin doesn't officially support RTL languages, using things like margin-left and margin-right are not compatible with RTL. Gap or logical properties are an easy solution to have it be bidirectional support.
Proposal
This would be considered a breaking change, as it will shift elements around and get rid of margins elements used to have. Ideally, I would like to see 0 margins used in the codebase. Anything that was previously using margin inside of a component for space should use BlockStack/InlineStack gap instead of margin. While that's highly unlikely, I think at the very least, two things need to happen:
There are some cases where components render margin if there is more than one rendered after each other. For example, Banners do this. I'd be open for discussion, but I think we should either:
a. Let the user render their own BlockStack if they have multiple
b. Provide a
BannerStack
component if we think it's important that the spacing is the same for all occurrences of this.Beta Was this translation helpful? Give feedback.
All reactions