Skip to content
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.

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

Svelte: compress progress indicator #62317

Merged
merged 3 commits into from
May 1, 2024
Merged

Svelte: compress progress indicator #62317

merged 3 commits into from
May 1, 2024

Conversation

camdencheek
Copy link
Member

@camdencheek camdencheek commented Apr 30, 2024

This removes some extra padding from the progress indicator and also simplifies some of the padding so it's symmetrical and doesn't rely on adding padding together to get the right layout.

Test plan

Before:
screenshot-2024-04-30_16-58-19@2x

After:
screenshot-2024-04-30_16-58-03@2x

@cla-bot cla-bot bot added the cla-signed label Apr 30, 2024
@fkling
Copy link
Contributor

fkling commented Apr 30, 2024

Maybe you want to tackle the rest of #62306 as part of this PR?

Base automatically changed from cc/fix-icon-size to main April 30, 2024 21:51
@@ -163,7 +163,7 @@
.progress-button {
border: 1px solid var(--border-color-2);
border-radius: 4px;
margin-left: 0.3rem;
padding: 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

Disable the default button padding so we can set it consistently without having to add paddings together

Comment on lines -72 to -73
min-width: 200px;
max-width: fit-content;
Copy link
Member Author

Choose a reason for hiding this comment

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

Got rid of the min-width and max-width because this looks better when the spacing is balanced.

Comment on lines -69 to +64
justify-content: space-evenly;
justify-content: space-between;
Copy link
Member Author

Choose a reason for hiding this comment

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

No functional difference, but since we are fitting to content, space-between is more semantically correct

@@ -37,17 +37,11 @@
</script>

<div class="indicator">
<div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Unnecessary outer div.

Comment on lines 31 to 41
width: $size;
height: $size;
&.inline {
$inlineSize: #{(16 / 14)}em;
width: $inlineSize;
height: $inlineSize;

vertical-align: bottom;
display: inline-flex;
align-items: center;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the ability to specify the size of the spinner and moved the inline styling into here because the .icon-inline styles expect to be styling an SVG.

width: $size;
height: $size;
&.inline {
$inlineSize: #{(16 / 14)}em;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to avoid confuse let's not use any scss variables.

@camdencheek camdencheek enabled auto-merge (squash) May 1, 2024 19:05
@camdencheek camdencheek merged commit 676f7c2 into main May 1, 2024
7 checks passed
@camdencheek camdencheek deleted the cc/compress-progress branch May 1, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants