Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/soft-clouds-shift.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react-brand': patch
---

Updated `SectionIntroStacked` layout spacing for improved visual alignment.
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,11 @@ export const Gridline: Story = {
)
},
}

export const GridlineTablet: Story = {
name: 'Gridline (tablet)',
globals: {
viewport: {value: 'ipad10p'},
},
render: Gridline.render,
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,26 @@
.SectionIntroStacked {
--brand-Grid-spacing-row: var(--base-size-48);
--brand-Grid-spacing-row: var(--base-size-32);
}

@media screen and (min-width: 48rem) {
.SectionIntroStacked {
--brand-Grid-spacing-row: var(--base-size-48);
}
}

@media screen and (min-width: 48rem) and (max-width: 63.24rem) {
.SectionIntroStacked {
max-width: 618px;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just curious whether we're accounting for some kind of inline borders here? Is that why it's not rounded up to 620px?

margin-inline: auto;
Comment on lines +7 to +8

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@rezrah I ended up using a max-width instead of the inline paddings, following @jesussandreas's comment on the QA ticket.

@jesussandreas, can you please confirm that this is the correct max-width for tablet viewports?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@danielguillan literally just seeing this. but it seems correct.

}

.SectionIntroStacked.SectionIntroStacked--variant-gridline {
padding-inline: var(--base-size-32);
padding-block: calc(var(--base-size-64) - var(--base-size-4));

--SectionIntroStacked-item-divider-overflow-start: var(--base-size-32);
--SectionIntroStacked-item-divider-overflow-end: var(--base-size-32);
}
Comment thread
danielguillan marked this conversation as resolved.
}

.SectionIntroStacked--variant-gridline {
Expand All @@ -21,15 +42,6 @@
}
}

@media screen and (min-width: 63.25rem) {
.SectionIntroStacked--variant-gridline {
padding-inline: var(--base-size-64);
padding-block: var(--base-size-64);
--SectionIntroStacked-item-divider-overflow-start: 0px;
--SectionIntroStacked-item-divider-overflow-end: calc(var(--brand-Grid-spacing-margin) + var(--base-size-64));
}
}

@media screen and (min-width: 80rem) {
.SectionIntroStacked--variant-gridline {
border-inline: var(--brand-borderWidth-thin) solid var(--brand-color-border-muted);
Expand Down Expand Up @@ -71,10 +83,13 @@
flex-direction: column;
}

@media screen and (max-width: 63.25rem) {
@media screen and (max-width: 63.24rem) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@danielguillan what was the reason for this breakpoint change? Want to understand the reason so we can consider fixing the inconsistencies in other components. We same to have more components using max-width of 63.25rem instead of 63.24rem right now.

.SectionIntroStacked-heading {
font-size: var(--brand-text-size-600);
}
.SectionIntroStacked-items {
overflow: hidden;
}
Comment thread
danielguillan marked this conversation as resolved.
}

@media screen and (min-width: 63.25rem) {
Expand All @@ -86,15 +101,14 @@
.SectionIntroStacked--variant-gridline .SectionIntroStacked-items {
border-inline-start: var(--brand-borderWidth-thin) solid var(--brand-color-border-muted);
margin-block: calc(-1 * var(--base-size-64));
padding-block: 0;
padding-block: calc(var(--base-size-64) - var(--base-size-32));
position: static;
top: auto;
}

.SectionIntroStacked--variant-gridline .SectionIntroStackedItem-item {
/* Inset item content from the vertical gridline */
padding-inline-start: var(--base-size-32);
padding-inline-end: var(--base-size-32);
padding-inline-start: var(--base-size-64);
}
}

Expand Down Expand Up @@ -137,14 +151,15 @@

.SectionIntroStackedItem-item--with-icon {
display: flex;
flex-direction: row;
align-items: flex-start;
gap: var(--base-size-24);
flex-direction: column;
}

@media screen and (min-width: 63.25rem) {
@media screen and (min-width: 48rem) {
.SectionIntroStackedItem-item--with-icon {
gap: var(--base-size-48);
flex-direction: row;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,9 @@ const Root = forwardRef<HTMLElement, PropsWithChildren<SectionIntroStackedProps>
{...props}
style={{...animationInlineStyles, ...style}}
>
<Grid fullWidth enableGutters={variant !== 'gridline'}>
<Grid fullWidth enableGutters={variant !== 'gridline'} columnGap="none">
<Grid.Column span={{large: 6}}>{otherChildren}</Grid.Column>
<Grid.Column
span={variant === 'gridline' ? {large: 6} : {large: 5}}
start={variant === 'gridline' ? {large: 7} : {large: 8}}
>
<Grid.Column span={{large: 6}} start={{large: 7}}>
{items}
</Grid.Column>
</Grid>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,19 @@ test.describe('Visual Comparison: SectionIntroStacked', () => {
await page.waitForTimeout(500)
await expect(page).toHaveScreenshot({fullPage: true})
})

// eslint-disable-next-line i18n-text/no-en
test.describe('Tablet viewport test for Gridline (tablet)', () => {
test.use({viewport: {width: 834, height: 1112}})
test('SectionIntroStacked / Gridline (tablet)', async ({page}) => {
await page.goto(
'http://localhost:6006/iframe.html?args=&id=components-sectionintrostacked-features--gridline-tablet&viewMode=story',
{waitUntil: 'networkidle'},
)
await page.locator('body.sb-show-main').waitFor({state: 'visible'})

await page.waitForTimeout(500)
await expect(page).toHaveScreenshot({fullPage: true})
})
})
})
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Comment thread
danielguillan marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@danielguillan @jesussandreas the space under the section intro is now really large on older templates like flex suite.. checkout this snapshot diff out for an example 👇.

Personally feel the spacing is too much even on gridline, but it looks dramatically worse on older termplates. Could/should we constrain or reduce it?

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export function FlexSuiteAIOverviewTemplate({content}: FlexSuiteAIOverviewTempla
<SectionIntro.Description>{content.workflow.description}</SectionIntro.Description>
</SectionIntro>
</Section>
<Section paddingBlockEnd="none">
<Section paddingBlockStart="condensed" paddingBlockEnd="none">
<Stack direction="vertical" padding="none" gap="spacious">
<River variant="gridline">
<River.Visual
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading