Skip to content

Update Layout's sidebar width values #1738

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

Merged
merged 4 commits into from
Nov 8, 2021
Merged

Update Layout's sidebar width values #1738

merged 4 commits into from
Nov 8, 2021

Conversation

vdepizzol
Copy link
Contributor

This update addresses some previous feedback on the Layout component's sidebar widths.

With the new values, the default sidebar width is more compact by default, being closer to its previous value. Originally a col-3 on a xl would render as ~280px. With the Layout component introduction it incorrectly increased to 320px. This PR dials it back to 296px.

On a desktop computer (xl breakpoint), these are the new options:

sidebarWidth Size
default 296px
narrow 256px
wide 344px

✌️

/cc @primer/css-reviewers @camertron @jonrohan

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@vdepizzol vdepizzol requested a review from a team as a code owner November 3, 2021 23:33
@changeset-bot
Copy link

changeset-bot bot commented Nov 3, 2021

🦋 Changeset detected

Latest commit: e05a6a0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

) !default;

$sidebar-narrow-width: (
md: 240px,
lg: 256px,
xl: 296px
lg: 256px
Copy link
Contributor

Choose a reason for hiding this comment

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

Sharing the output from testing for clarity:

@media (min-width: 768px) {
  .Layout.Layout--sidebar-narrow {
    --Layout-sidebar-width: 240px;
  }
}
@media (min-width: 1012px) {
  .Layout.Layout--sidebar-narrow {
    --Layout-sidebar-width: 256px;
  }
}

@@ -169,14 +169,12 @@ $responsive-positions: (
$sidebar-width: (
sm: 220px,
md: 256px,
lg: 296px,
xl: 320px
lg: 296px
Copy link
Contributor

Choose a reason for hiding this comment

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

Output:

@media (min-width: 544px) {
  .Layout {
    --Layout-sidebar-width: 220px;
  }
}
@media (min-width: 768px) {
  .Layout {
    --Layout-sidebar-width: 256px;
  }
}
@media (min-width: 1012px) {
  .Layout {
    --Layout-sidebar-width: 296px;
  }
}

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

@jonrohan jonrohan enabled auto-merge (squash) November 8, 2021 19:22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jonrohan jonrohan merged commit dfe73db into main Nov 8, 2021
@jonrohan jonrohan deleted the layout-sidebar-width branch November 8, 2021 19:24
@primer-css primer-css mentioned this pull request Nov 8, 2021
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