Skip to content

[Banner] Rebuild with layout components #7365

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

Merged
merged 10 commits into from
Oct 18, 2022
Merged

[Banner] Rebuild with layout components #7365

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4e614dc
Refactor `Banner` with layout components
aveline Oct 6, 2022
a8e2143
Refactor `Banner` actions styles
aveline Oct 6, 2022
1b385ce
Update Banner.test.tsx
aveline Oct 6, 2022
a5af2a3
Remove unused const
aveline Oct 6, 2022
08de3f6
changeset
aveline Oct 6, 2022
72cdd60
Remove unused import
aveline Oct 6, 2022
72dcd5d
👕 Remove needless disable for "selector-max-combinators"
aveline Oct 6, 2022
bbbd721
Refactor `AccountConnection` with new layout and type components (#7345)
aveline Oct 12, 2022
88856f7
Merge branch 'main' into layout-components-beta
aveline Oct 12, 2022
7ee1f68
Merge branch 'layout-components-beta' into layout-banner
aveline Oct 12, 2022
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/healthy-apricots-rule.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Updated `Banner` component to use new layout primitives
46 changes: 6 additions & 40 deletions polaris-react/src/components/Banner/Banner.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@
position: relative;
display: flex;

// stylelint-disable selector-max-class, selector-max-combinators, selector-max-specificity
&.statusCritical .PrimaryAction .Button {
// stylelint-disable selector-max-class, selector-max-specificity
&.statusCritical .PrimaryAction.Button {
border-color: var(--p-border-critical-subdued);
background: var(--p-surface-critical-subdued);

Expand All @@ -75,7 +75,7 @@
}
}

&.statusWarning .PrimaryAction .Button {
&.statusWarning .PrimaryAction.Button {
border-color: var(--p-border-warning-subdued);
background: var(--p-surface-warning-subdued);

Expand All @@ -95,7 +95,7 @@
}
}

&.statusInfo .PrimaryAction .Button {
&.statusInfo .PrimaryAction.Button {
border-color: var(--p-border-highlight-subdued);
background: var(--p-surface-highlight-subdued);

Expand All @@ -115,7 +115,7 @@
}
}

&.statusSuccess .PrimaryAction .Button {
&.statusSuccess .PrimaryAction.Button {
border-color: var(--p-border-success-subdued);
background: var(--p-surface-success-subdued);

Expand All @@ -134,12 +134,7 @@
background: var(--p-surface-success-subdued);
}
}
// stylelint-enable selector-max-class, selector-max-combinators, selector-max-specificity
}

.ContentWrapper {
margin-top: calc(-1 * var(--p-space-05));
flex: 1 1 auto;
// stylelint-enable selector-max-class, selector-max-specificity
}

.withinContentContainer {
Expand All @@ -151,19 +146,11 @@
position: absolute;
}

.Ribbon {
padding-right: var(--p-space-4);
}

@include banner-variants($in-page: false);

+ .Banner {
margin-top: var(--p-space-2);
}

.Actions {
padding: var(--p-space-3) 0 var(--p-space-1) 0;
}
}

.withinPage {
Expand All @@ -180,10 +167,6 @@
padding-right: var(--p-space-4);
}

.Actions {
padding-top: var(--p-space-4);
}

.Dismiss {
right: var(--p-space-4);
top: var(--p-space-5);
Expand All @@ -195,23 +178,6 @@
padding-right: calc(var(--p-space-8) + var(--p-icon-size-small));
}

.Heading {
word-break: break-word;
}

.Content {
@include text-breakword;
padding: var(--p-space-05) 0;
}

.Ribbon {
flex: 0 0 var(--p-space-8);
}

.PrimaryAction {
margin-right: var(--p-space-2);
}

// We need pretty high specificity to do the descendant selectors
// onto the text, which needs to be the relative positioned wrapper
// so that the borders/ backgrounds do not extend outside of it.
Expand Down
43 changes: 20 additions & 23 deletions polaris-react/src/components/Banner/Banner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,18 @@ import {

import {classNames, variationName} from '../../utilities/css';
import {BannerContext} from '../../utilities/banner-context';
import {useUniqueId} from '../../utilities/unique-id';
import {useI18n} from '../../utilities/i18n';
import type {Action, DisableableAction, LoadableAction} from '../../types';
import {Button} from '../Button';
import {Heading} from '../Heading';
import {ButtonGroup} from '../ButtonGroup';
import {UnstyledButton, unstyledButtonFrom} from '../UnstyledButton';
import {UnstyledLink} from '../UnstyledLink';
import {Spinner} from '../Spinner';
import {Icon, IconProps} from '../Icon';
import {WithinContentContext} from '../../utilities/within-content-context';
import {Text} from '../Text';
import {Box} from '../Box';
import {Bleed} from '../Bleed';

import styles from './Banner.scss';

Expand Down Expand Up @@ -64,7 +65,6 @@ export const Banner = forwardRef<BannerHandles, BannerProps>(function Banner(
bannerRef,
) {
const withinContentContainer = useContext(WithinContentContext);
const id = useUniqueId('Banner');
const i18n = useI18n();
const {wrapperRef, handleKeyUp, handleBlur, handleMouseUp, shouldShowFocus} =
useBannerFocus(bannerRef);
Expand All @@ -79,14 +79,12 @@ export const Banner = forwardRef<BannerHandles, BannerProps>(function Banner(
);

let headingMarkup: React.ReactNode = null;
let headingID: string | undefined;

if (title) {
headingID = `${id}Heading`;
headingMarkup = (
<div className={styles.Heading} id={headingID}>
Copy link
Member

@kyledurand kyledurand Oct 7, 2022

Choose a reason for hiding this comment

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

Removing id is likely a breaking change. I imagine this was added for analytics or tracking of some kind. Makes me a bit uncomfortable to remove it but I want to

Copy link
Contributor Author

@aveline aveline Oct 7, 2022

Choose a reason for hiding this comment

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

Oh that's a good call about analytics tracking. The thinking in this case was the id was maybe used for semantics and aria labelling. I do have another PR to add id and could refactor this if we are game to merge that in #7363

Copy link
Member

@alex-page alex-page Oct 11, 2022

Choose a reason for hiding this comment

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

There is no way for teams to know the ID as it's generated with useUniqueId. I think this should be safe to remove. Other ID's though this could be the case.

Copy link
Member

@kyledurand kyledurand Oct 13, 2022

Choose a reason for hiding this comment

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

@BPScott do you remember why banners needed unique ids?

Copy link
Member

@alex-page alex-page Oct 17, 2022

Choose a reason for hiding this comment

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

It is because they don't use semantic heading elements so they get aria roles to link the ID. This should be fixed with HTML.

<Heading element="p">{title}</Heading>
</div>
<Text as="h2" variant="headingMd">
{title}
</Text>
);
}

Expand All @@ -109,13 +107,13 @@ export const Banner = forwardRef<BannerHandles, BannerProps>(function Banner(
) : null;

const primaryActionMarkup = action ? (
<div className={styles.PrimaryAction}>
<Box paddingRight="2">
{action.loading
? spinnerMarkup
: unstyledButtonFrom(action, {
className: styles.Button,
className: `${styles.Button} ${styles.PrimaryAction}`,
})}
</div>
</Box>
) : null;

const secondaryActionMarkup = secondaryAction ? (
Expand All @@ -124,24 +122,25 @@ export const Banner = forwardRef<BannerHandles, BannerProps>(function Banner(

const actionMarkup =
action || secondaryAction ? (
<div className={styles.Actions}>
<Box
paddingTop={withinContentContainer ? '3' : '4'}
paddingBottom={withinContentContainer ? '1' : undefined}
>
<ButtonGroup>
{primaryActionMarkup}
{secondaryActionMarkup}
</ButtonGroup>
</div>
</Box>
) : null;

let contentMarkup: React.ReactNode = null;
let contentID: string | undefined;

if (children || actionMarkup) {
contentID = `${id}Content`;
contentMarkup = (
<div className={styles.Content} id={contentID}>
<Box paddingTop="05" paddingBottom="05">
Copy link
Member

@kyledurand kyledurand Oct 7, 2022

Choose a reason for hiding this comment

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

Wondering if we could do something like this until we can remove id:

Suggested change
<Box paddingTop="05" paddingBottom="05">
<Box paddingTop="05" paddingBottom="05">
<div id={contentID} />

We'd have to ping the person who contributed the id prop to the component to see if this would be sufficient

Copy link
Contributor Author

@aveline aveline Oct 7, 2022

Choose a reason for hiding this comment

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

Might be some context here #3199

{children}
{actionMarkup}
</div>
</Box>
);
}

Expand All @@ -168,19 +167,17 @@ export const Banner = forwardRef<BannerHandles, BannerProps>(function Banner(
onMouseUp={handleMouseUp}
onKeyUp={handleKeyUp}
onBlur={handleBlur}
aria-labelledby={headingID}
aria-describedby={contentID}
>
{dismissButton}

<div className={styles.Ribbon}>
<Box paddingRight="4">
<Icon source={iconName} color={iconColor} />
</div>
</Box>

<div className={styles.ContentWrapper}>
<Bleed top="05">
{headingMarkup}
{contentMarkup}
</div>
</Bleed>
</div>
</BannerContext.Provider>
);
Expand Down
8 changes: 4 additions & 4 deletions polaris-react/src/components/Banner/tests/Banner.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import {mountWithApp} from 'tests/utilities';

import {Button} from '../../Button';
import {Heading} from '../../Heading';
import {Text} from '../../Text';
import {Icon} from '../../Icon';
import {Spinner} from '../../Spinner';
import {UnstyledButton} from '../../UnstyledButton';
Expand All @@ -21,12 +21,12 @@ import {Banner, BannerHandles} from '../Banner';
describe('<Banner />', () => {
it('renders a title', () => {
const banner = mountWithApp(<Banner title="Banner title" />);
expect(banner.find(Heading)).toContainReactText('Banner title');
expect(banner.find(Text)).toContainReactText('Banner title');
});

it('passes a p element to Heading', () => {
it('passes an h2 element to Heading', () => {
const banner = mountWithApp(<Banner title="Banner title" />);
expect(banner).toContainReactComponent(Heading, {element: 'p'});
expect(banner).toContainReactComponent(Text, {as: 'h2'});
});

it('passes the provided icon source to Icon', () => {
Expand Down