Skip to content

[diffs] Remove necessary set shorthands and defaults #741

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.

Sign up for GitHub

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

Open
Que-tin wants to merge 1 commit into
base: main
Choose a base branch
from
+10 −14
Open

[diffs] Remove necessary set shorthands and defaults #741

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions packages/diffs/src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@
margin: 0;
padding: 0;
display: block;
outline: none;
outline-style: none;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Literally the same, without other properties being set under the hood

font-family: var(--diffs-font-family, var(--diffs-font-fallback));
}

Expand Down Expand Up @@ -811,7 +811,7 @@
}

[data-code]::-webkit-scrollbar-track {
background: transparent;
background-color: transparent;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Literally the same, without other properties being set under the hood.

Also already commonly used across the file

}

[data-code]::-webkit-scrollbar-thumb {
Expand Down Expand Up @@ -1048,7 +1048,6 @@
[data-expand-button],
[data-separator-content] {
display: flex;
flex: 0 0 auto;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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)

align-items: center;
background-color: var(--diffs-bg-separator);
}
Expand Down Expand Up @@ -1081,7 +1080,7 @@
}

[data-separator-content] {
flex: 1 1 auto;
flex-grow: 1;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same as above

padding: 0 1ch;
height: 100%;
color: var(--diffs-fg-number);
Expand All @@ -1105,7 +1104,6 @@
min-width: 0;
text-overflow: ellipsis;
white-space: nowrap;
flex: 0 1 auto;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

These are basically the UA-defaults

@amadeus amadeus Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

}

@supports (width: 1cqi) {
Expand Down Expand Up @@ -1525,7 +1523,7 @@
[data-merge-conflict-actions-content] {
display: flex;
align-items: center;
gap: 0.25rem;
column-gap: 0.25rem;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have a default flexbox no need to set row-gap as well

padding-inline: 0.5rem;
min-height: 1.75rem;
font-family: var(
Expand All @@ -1539,8 +1537,8 @@

[data-merge-conflict-action] {
appearance: none;
border: 0;
background: transparent;
border-style: none;
background-color: transparent;
Comment on lines +1540 to +1541

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same as at the start

color: var(--diffs-fg-number);
font: inherit;
font-style: normal;
Expand Down Expand Up @@ -1572,10 +1570,9 @@
position: relative;
background-color: var(--diffs-bg);
display: flex;
flex-direction: row;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

UA-Default

@amadeus amadeus Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this probably isn't needed, but i am a fan of seeing the direction listed out here to help establish context.

@Que-tin Que-tin Jun 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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. 😊

@amadeus amadeus Jun 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

justify-content: space-between;
align-items: center;
gap: var(--diffs-gap-inline, var(--diffs-gap-fallback));
column-gap: var(--diffs-gap-inline, var(--diffs-gap-fallback));

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have a default flexbox no need to set row-gap as well

min-height: calc(
1lh + (var(--diffs-gap-block, var(--diffs-gap-fallback)) * 3)
);
Expand All @@ -1586,9 +1583,8 @@

[data-header-content] {
display: flex;
flex-direction: row;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

UA-Default

@amadeus amadeus Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar note above, I personally prefer the context here to help establish what's happening with layout.

align-items: center;
gap: var(--diffs-gap-inline, var(--diffs-gap-fallback));
column-gap: var(--diffs-gap-inline, var(--diffs-gap-fallback));

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have a default flexbox no need to set row-gap as well

min-width: 0;
white-space: nowrap;
}
Expand All @@ -1615,7 +1611,7 @@
[data-diffs-header='default'] [data-metadata] {
display: flex;
align-items: center;
gap: 1ch;
column-gap: 1ch;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have a default flexbox no need to set row-gap as well

white-space: nowrap;
}

Expand Down Expand Up @@ -1696,7 +1692,7 @@
display: flex;
align-items: center;
justify-content: center;
border: none;
border-style: none;

@Que-tin Que-tin May 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Same as with outline -> outline-style

appearance: none;
width: 1lh;
height: 1lh;
Expand Down
Loading