twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
GNU Affero General Public License v3.0
17.36k stars 2.04k forks source link

`box-sizing` issues can arise out of the blue #7741

Open Devessier opened 2 days ago

Devessier commented 2 days ago

I was creating a new button in a confirmation modal and chose to make it a link. The link was not appropriately positioned, even though it had the same styles as the buttons below and above. Therefore, the styling difference was due to the HTML elements' nature and the way the browser styles them by default.

CleanShot 2024-10-16 at 11 24 08@2x

By default, the box-sizing property equals content-box for links and border-box for buttons. The difference is how the browser calculates the element's actual width when we set an explicit width (like width: 100px or width: 100%).

With content-box, the browser adds the padding and the element's border to the width. Settings width: 100px; padding: 16px; border: 4px solid black; will make an element 140px large (100px + 2 * 16px + 2 * 4px). See the MDN's article on this property.

Most modern CSS solutions set the box-sizing property of all elements and pseudo-elements to box-sizing, which is much easier to think about. For instance, that's the choice made by modern-normalize, the CSS reset library used by Tailwind CSS.

I saw that the box-sizing property is set to border-box many times in the app. We might want to consider making it global, which would prevent nasty bugs like the one I found.

CleanShot 2024-10-16 at 11 32 41@2x

Here is an article explaining why that's a good idea to do so: https://www.paulirish.com/2012/box-sizing-border-box-ftw/

And here's the CSS to add: https://github.com/sindresorhus/modern-normalize/blob/3faa07fc91b8ffbe6243bc4f91e880a00185d0a2/modern-normalize.css#L8-L16.

If we choose to go this way, we'll have to ensure it doesn't introduce regressions where we rely on the content-box behavior.

That's a choice we already made for the chrome extension and the website:

charlesBochet commented 2 days ago

Generally, I'm in favor of having a consensus in the project, so I would advocate to update our project guidelines to be:

Once this is validated (let's see if @lucasbordeau and the rest of the team), we can discuss how to enforce it:

I would prefer (b) if inheritance work, otherwise (a) seems reasonable with a good UX

Devessier commented 1 day ago

The box-sizing property is not inherited, as per the specification found on MDN:

CleanShot 2024-10-17 at 15 22 23@2x

I don't advocate for global selectors, but that would be the most straightforward way to apply this style to every element. I'm happy to discuss any more emotion-ish way of doing it.

lucasbordeau commented 1 day ago

Would require more in depth thinking on my side, but what I can already say is that I don't see why we would need to apply a solution which has a global CSS impact for a DX problem that just begins to rise refactoring questions on a case-by-case basis. To me it seems a bit impulsive and would require more detailed real cases in our application.

I would also say : be careful with hype driven development, Tailwind CSS is surely a well known library, but nothing new under the sun in frontend development with the stack we have, since we use a from scratch approach with our components, I don't see why we would need to mimic any CSS library since they have different concerns than us. Libraries strive to provide an opinionated way of doing CSS which has its advantages and disadvantages

From modern-normalize own words in their codebase : Use a better box model (opinionated).

So general feeling : since we're not in a hurry, let's see first how we can solve this for our buttons ?

Devessier commented 1 day ago

Thanks for your thorough answer, @lucasbordeau!

I don't advocate for a quick change. As you said, setting the box-sizing property globally could have huge impacts on many different places in the application, and much care would have to go into this change. We can give ourselves time to think about the problem. I'll be happy if we find an easy fix for now!

The box-sizing property is not well-known, and the content-box value doesn't feel intuitive. It's possible that it took the team and contributors a lot of time to find the correct fix for the 50 times the box-sizing property had to be explicitly set to border-box in the project. That's why I call it nasty. CSS is hard enough; I like when it's made simpler 😄

Sure, we hear a lot about Tailwind CSS. Still, it was created roughly at the same time as Emotion, the css-in-js solution we use in the project 😄 I've been using Tailwind since 2019 and find it great. Some popular tools are actually good, like React 😁 I didn't mean that we had to mimic the behavior of a library, only that they made an interesting choice from which we could take inspiration—if we chose to!

To summarize, here is a suggestion for the following steps:

@charlesBochet, what do you think?

charlesBochet commented 1 day ago

Let's discuss it together tomorrow, I think that the Figma designs are mostly based on content-box so we should actually follow it to simplify design integrations. In practice, making the box-sizing applied to everything might be tricky (a lot of elements will likely change their width) and I am not a big fan of implicit behaviors but I could live with this one :)

lucasbordeau commented 21 hours ago

For me the 100% sure choice that cannot fail us is to apply this only to a type of component or to some broadly used components like Buttons to see what happens and if we can expand this and how.

Let's start small, and see from there. If we find out that it's easy to just use * border-box, why not ? But the risk is 100% lower if we start small, without drawback.

lucasbordeau commented 21 hours ago

Also I agree that there maybe another path : educate devs on this CSS with a best-practice and describing the cases where it applies and not, instead of doing it implicitly for everyone.

Another question we should answer : what would be the cases where we wouldn't want to have content-box ?

lucasbordeau commented 21 hours ago

Also for Figma : if they took a certain decision about it for their own reason, maybe it would be risky to also mimic them because we could end up mimicking something that equals a bad practice decision for us.

Plus this integration problem is something they are currently working on because that's an open problem with their current product and maybe we shouldn't fix our codebase on their current product also but find a way to maybe educate our devs on how to easily integrate Figma design with our CSS decisions ? So that we stay Figma-agnostic ?