[diffs] Remove necessary set shorthands and defaults#741
Conversation
|
@Que-tin is attempting to deploy a commit to the Pierre Computer Company Team on Vercel. A member of the Team first needs to authorize it. |
| padding: 0; | ||
| display: block; | ||
| outline: none; | ||
| outline-style: none; |
There was a problem hiding this comment.
Literally the same, without other properties being set under the hood
|
|
||
| [data-code]::-webkit-scrollbar-track { | ||
| background: transparent; | ||
| background-color: transparent; |
There was a problem hiding this comment.
Literally the same, without other properties being set under the hood.
Also already commonly used across the file
| [data-expand-button], | ||
| [data-separator-content] { | ||
| display: flex; | ||
| flex: 0 0 auto; |
There was a problem hiding this comment.
This was unnecessary.
flex-grow: 0 is the default.
Same goes with flex-basis: auto.
[data-expand-button] was already setting flex-shrink: 0 individually
[data-separator-content] was overriding the whole thing anyway (see next comment)
|
|
||
| [data-separator-content] { | ||
| flex: 1 1 auto; | ||
| flex-grow: 1; |
| min-width: 0; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| flex: 0 1 auto; |
There was a problem hiding this comment.
These are basically the UA-defaults
There was a problem hiding this comment.
I personally prefer to see these types of flex values in context because you don't have to remember ua defaults and you can see what is being defined explicitly.
| display: flex; | ||
| align-items: center; | ||
| gap: 1ch; | ||
| column-gap: 1ch; |
There was a problem hiding this comment.
We have a default flexbox no need to set row-gap as well
| align-items: center; | ||
| justify-content: center; | ||
| border: none; | ||
| border-style: none; |
There was a problem hiding this comment.
Same as with outline -> outline-style
|
|
||
| [data-header-content] { | ||
| display: flex; | ||
| flex-direction: row; |
There was a problem hiding this comment.
Similar note above, I personally prefer the context here to help establish what's happening with layout.
| position: relative; | ||
| background-color: var(--diffs-bg); | ||
| display: flex; | ||
| flex-direction: row; |
There was a problem hiding this comment.
I know this probably isn't needed, but i am a fan of seeing the direction listed out here to help establish context.
There was a problem hiding this comment.
Interesting. Will revert the changes then.
Out of curiosity where do you draw the line?
I mean you also don't define display: block or inline on each element or the font-sizes, weights and family. What part of the cascade or UAs is "worth" ignoring and which part is worth redefining?
Really interested to get some insights in the thought process behind this, never stumbled across such pattern. 😊
There was a problem hiding this comment.
There’s no hard and fast rule I have here, some of it stems from my experience working in react native where the default direction is column. There’s likely inconsistencies in this file already to begin with, but if intentional changes are happening I would prefer explicitness over ambiguity.
| border-style: none; | ||
| background-color: transparent; |
amadeus
left a comment
amadeus
left a comment
There was a problem hiding this comment.
Most of these I'm good with, but I do have a bit of a stylistic preference in being more explicit around flex properties. So ideally we keep those but the rest look good to me. Given this PR was opened a while ago (sorry for the delays, a lot going on), you might need a rebase to catch up and it might involve some additional changes.
| position: relative; | ||
| background-color: var(--diffs-bg); | ||
| display: flex; | ||
| flex-direction: row; |
There was a problem hiding this comment.
I know this probably isn't needed, but i am a fan of seeing the direction listed out here to help establish context.
|
|
||
| [data-header-content] { | ||
| display: flex; | ||
| flex-direction: row; |
There was a problem hiding this comment.
Similar note above, I personally prefer the context here to help establish what's happening with layout.
| min-width: 0; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| flex: 0 1 auto; |
There was a problem hiding this comment.
I personally prefer to see these types of flex values in context because you don't have to remember ua defaults and you can see what is being defined explicitly.
amadeus
changed the title
Will look into it as soon as I'm back from conference. 👍🏻 |
2 participants
Description
This is the first of a couple isolated PRs to improve the stylings of diffs.
It removes necessary shorthands that under the hood set or reset way too many properties that aren't even used and also removes a couple of stylings that I stumbled upon that are user agent defaults across browsers anyways.
I'll comment each individual change in the PR, check there for more info
Motivation & Context
Type of changes
first discussed with the dev team and they should be aware that this PR is
being opened
You must have first discussed with the dev team and they should be aware
that this PR is being opened
Checklist
contributing guidelines
bun run lint)bun run format)bun run diffs:test)How was AI used in generating this PR
Not at all.