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.

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

Fix/container spacing #71

Merged
merged 3 commits into from
Sep 26, 2023
Merged

Fix/container spacing #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adb9891
fix: fluid-container padding size on mobile
dannyk3941 Sep 22, 2023
460c192
fix: replace values in _application.scss with variables
dannyk3941 Sep 22, 2023
eb1fccf
fix: remove upper limit in _container.scss media query
dannyk3941 Sep 26, 2023
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
13 changes: 6 additions & 7 deletions packages/vanilla/src/sass/components/header/_application.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,18 @@
@mixin menu-hover {
color: var(--cbp-color-text-darkest);
background-color: var(--cbp-color-interactive-secondary-lighter);
//border-style: solid;
border-color: var(--cbp-color-interactive-secondary-darker);
//border-width: 0 0 var(--cbp-border-size-md) 0;
}

.cbp-application-header {
display: flex;
align-items: center;
height: 56px;
height: var(--cbp-space-14x);
width: 100%;
position: sticky;
top: -1px;
top: calc(-1 * var(--cbp-space-half-x));
background-color: var(--cbp-color-white);
box-shadow: 0px 5px 5px -3px var(--cbp-color-interactive-secondary-dark);
box-shadow: var(--cbp-shadow-level-1-down);
padding-right: var(--cbp-space-4x);
z-index: var(--cbp-z-index-level-1);

Expand Down Expand Up @@ -62,7 +60,7 @@
border-width: 0 0 var(--cbp-border-size-md) 0;

& > i {
margin-left: 8px;
margin-left: var(--cbp-space-2x);
}

&:hover {
Expand All @@ -82,7 +80,8 @@
.nav-home {
font-size: var(--cbp-font-size-heading-sm);
font-weight: var(--cbp-font-weight-bold);
padding: 0 var(--cbp-space-4x) 0 var(--cbp-space-4x);
padding-left: var(--cbp-space-5x);
padding-right: var(--cbp-space-5x);
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/vanilla/src/sass/layout/_container.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
@use './../tokens/breakpoints' as *;

.cbp-fluid-container {
padding-left: var(--cbp-space-4x);
padding-right: var(--cbp-space-4x);
padding-left: var(--cbp-space-5x);
padding-right: var(--cbp-space-5x);
Copy link
Collaborator

Choose a reason for hiding this comment

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

20px seems like a very large amount of padding for a potentially small device. If this is the spec, I think we need to discuss it further. I typically see very little padding at the mobile sizes and use .5rem tops myself. 40px is 10% (or more) of the screen width on a phone.


@media (min-width: $cbp-breakpoint-md) and (max-width: $cbp-breakpoint-xl) {
@media (min-width: $cbp-breakpoint-md) {
padding-left: var(--cbp-space-8x);
padding-right: var(--cbp-space-8x);
}
Expand Down