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

fix: Prevent focus ring from getting cut off in Popup.Body #1727

Merged
merged 47 commits into from Sep 9, 2022
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
00b824a
Merge branch 'prerelease/minor' of https://github.com/Workday/canvas-…
Jul 18, 2022
10135cf
Merge branch 'prerelease/minor' of https://github.com/Workday/canvas-…
Aug 11, 2022
a81c9d5
fix: Prevent focus ring from getting cut off in modals
Aug 11, 2022
e146979
fix: Clean up storybook examples
Aug 11, 2022
646652d
Merge branch 'prerelease/minor' into mc-fix-modal-layout
mannycarrera4 Aug 11, 2022
7cd9e13
fix: Remove unused imports
Aug 11, 2022
2a88f10
Merge branch 'mc-fix-modal-layout' of https://github.com/mannycarrera…
Aug 11, 2022
2389c8a
fix: Need to fix cypress
Aug 11, 2022
2e4a4ef
test: Update cypress test to match stories
Aug 11, 2022
a73de46
test: Update tests to match stories
Aug 12, 2022
8407f75
fix: Remove stack from popup card
Aug 12, 2022
aa24bcd
fix: Revert story changes
Aug 15, 2022
c81291c
Merge branch 'prerelease/minor' into mc-fix-modal-layout
mannycarrera4 Aug 15, 2022
5c44fb1
fix: Update padding
Aug 15, 2022
05744a3
fix: Add back in spacing
Aug 15, 2022
499b214
Merge branch 'prerelease/minor' into mc-fix-modal-layout
mannycarrera4 Aug 22, 2022
cce81dc
fix: Clean up modal padding
Aug 24, 2022
cf78dbc
Merge branch 'mc-fix-modal-layout' of https://github.com/mannycarrera…
Aug 24, 2022
e2dd3ee
fix: Clean up modal padding
Aug 24, 2022
3484d99
fix: Clean up remaining padding
Aug 24, 2022
901a032
Merge branch 'prerelease/minor' into mc-fix-modal-layout
mannycarrera4 Aug 24, 2022
2838042
fix: Move logic to popup
Aug 24, 2022
f507f05
Merge branch 'mc-fix-modal-layout' of https://github.com/mannycarrera…
Aug 24, 2022
880dab0
fix: Clean up toast and popup stories
Aug 26, 2022
78797d4
fix: Remove flex and use stack
Aug 26, 2022
8311a2a
fix: Clean up popup card props
Aug 26, 2022
c45a6e7
fix: Clean up stories and remove footer for popup and modal
Aug 30, 2022
9f33843
fix: Clean up stories function name
Aug 31, 2022
b463883
fix: Clean up console message
Aug 31, 2022
f1a2e23
fix: Clean up imports
Aug 31, 2022
4424cfa
fix: Add back in spacing
Aug 31, 2022
5446425
fix: Add some maring to heading
Sep 6, 2022
a3ed5a3
fix: Update spacing between heading and body
Sep 7, 2022
1be9adf
fix: Clean up padding, thanks Alan and James
Sep 8, 2022
5923f19
Merge branch 'master' into mc-fix-modal-layout
mannycarrera4 Sep 8, 2022
c880109
fix: Clean up padding
Sep 8, 2022
6b6224d
Merge branch 'mc-fix-modal-layout' of https://github.com/mannycarrera…
Sep 8, 2022
baff697
fix: Clean up more code
Sep 8, 2022
053db05
fix: Update stories with correct padding
Sep 8, 2022
d86c066
fix: Clean up toast padding
Sep 8, 2022
7bebed7
fix: Clean up visual testing story
Sep 8, 2022
6f4beba
fix: Clean up dialog story
Sep 8, 2022
9a7f498
fix: Move HStack footer out of Body for Popup stories
jamesfan Sep 8, 2022
2056502
fix: Remove primary button from example
Sep 8, 2022
9f2a8a1
Merge branch 'mc-fix-modal-layout' of https://github.com/mannycarrera…
Sep 8, 2022
ed58723
Update modules/react/popup/lib/PopupCard.tsx
mannycarrera4 Sep 8, 2022
de29d1c
fix: Fix paragraph margin on Popup RTL example
jamesfan Sep 8, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/react/modal/stories/examples/Basic.tsx
Expand Up @@ -17,7 +17,7 @@ export const Basic = () => {
<Modal.CloseIcon aria-label="Close" />
<Modal.Heading>Delete Item</Modal.Heading>
<Modal.Body>
<Box as="p" marginTop={0} marginBottom="m">
<Box as="p" marginTop="zero" marginBottom="m">
jamesfan marked this conversation as resolved.
Show resolved Hide resolved
Are you sure you want to delete the item?
</Box>
<HStack spacing="s">
Expand Down
7 changes: 3 additions & 4 deletions modules/react/modal/stories/examples/BodyOverflow.tsx
Expand Up @@ -2,8 +2,7 @@ import React from 'react';

import {Modal} from '@workday/canvas-kit-react/modal';
import {DeleteButton} from '@workday/canvas-kit-react/button';
import {HStack, VStack, Stack, Box} from '@workday/canvas-kit-react/layout';
import {space} from '@workday/canvas-kit-react/tokens';
import {HStack} from '@workday/canvas-kit-react/layout';

export const BodyOverflow = () => {
const handleDelete = () => {
Expand All @@ -17,7 +16,7 @@ export const BodyOverflow = () => {
<Modal.Card>
<Modal.CloseIcon aria-label="Close" />
<Modal.Heading>Delete Item</Modal.Heading>
<Modal.Body>
<Modal.Body paddingBottom="zero">
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
Expand All @@ -43,7 +42,7 @@ export const BodyOverflow = () => {
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
</Modal.Body>
<HStack spacing="s" paddingTop="s">
<HStack spacing="s" paddingTop="s" paddingX="l" paddingBottom="l">
Copy link
Member

Choose a reason for hiding this comment

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

Now that we've removed Modal.Footer, it feels odd to me to require consumers to know they need paddingX="l" and paddingBottom="l" to get their HStack Footer to render with complementary padding to match Modal.Body and Modal.Heading. Previously, this was padding was baked into Modal.Footer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we make this a Minor release, it makes sense to add this back in. But I agree the consumer would have to know what padding to add

Copy link
Member

Choose a reason for hiding this comment

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

For now, the consumer will need to add their own footer (using an HStack, for instance) with the appropriate paddings and margins to align with the Heading and Body subcomponents (see screenshot in top-level PR comment). We'll update Popup, Modal, and Dialog examples to include example footers with the needed padding and margin.

<Modal.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
</Modal.CloseButton>
Expand Down
4 changes: 2 additions & 2 deletions modules/react/modal/stories/examples/FullOverflow.tsx
Expand Up @@ -16,7 +16,7 @@ export const FullOverflow = () => {
<Modal.Card maxHeight="inherit" height="inherit">
<Modal.CloseIcon aria-label="Close" />
<Modal.Heading>Delete Item</Modal.Heading>
<Modal.Body>
<Modal.Body paddingBottom="zero">
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
Expand All @@ -42,7 +42,7 @@ export const FullOverflow = () => {
<p>Are you sure you want to delete the item?</p>
<p>Are you sure you want to delete the item?</p>
</Modal.Body>
<HStack spacing="s">
<HStack spacing="s" paddingX="l" paddingBottom="l">
<Modal.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
</Modal.CloseButton>
Expand Down
20 changes: 9 additions & 11 deletions modules/react/modal/stories/stories_testing.tsx
Expand Up @@ -101,17 +101,15 @@ export const WithRadioButtons = () => {
<Modal.CloseIcon aria-label="Close" />
<Modal.Heading>Select Item</Modal.Heading>
<Modal.Body>
<VStack spacing="s">
<RadioGroup
name="contact"
data-testid="radiogroup"
value={value}
onChange={value => setValue(String(value))}
>
<Radio id="1" value="email" label="E-mail" />
<Radio id="2" value="phone" label="Phone" />
</RadioGroup>
</VStack>
<RadioGroup
name="contact"
data-testid="radiogroup"
value={value}
onChange={value => setValue(String(value))}
>
<Radio id="1" value="email" label="E-mail" />
<Radio id="2" value="phone" label="Phone" />
</RadioGroup>
</Modal.Body>
</Modal.Card>
</Modal.Overlay>
Expand Down
4 changes: 3 additions & 1 deletion modules/react/popup/lib/PopupBody.tsx
Expand Up @@ -8,5 +8,7 @@ export const PopupBody = createSubcomponent('div')({
displayName: 'Popup.Body',
modelHook: usePopupModel,
})<ExtractProps<typeof Card.Body>>(elemProps => {
return <Card.Body overflowY="auto" {...elemProps} />;
return (
<Card.Body overflowY="auto" paddingX={'l'} paddingTop="s" paddingBottom="l" {...elemProps} />
jamesfan marked this conversation as resolved.
Show resolved Hide resolved
);
});
3 changes: 1 addition & 2 deletions modules/react/popup/lib/PopupCard.tsx
Expand Up @@ -16,7 +16,6 @@ import {Stack, StackStyleProps} from '@workday/canvas-kit-react/layout';

import {getTransformFromPlacement} from './getTransformFromPlacement';
import {usePopupCard, usePopupModel} from './hooks';

export interface PopupCardProps extends ExtractProps<typeof Card, never>, Partial<StackStyleProps> {
children?: React.ReactNode;
}
Expand Down Expand Up @@ -71,12 +70,12 @@ export const PopupCard = createSubcomponent('div')({
as={As}
transformOrigin={transformOrigin}
position="relative"
padding="l"
depth={5}
maxWidth={`calc(100vw - ${space.l})`}
spacing={0}
jamesfan marked this conversation as resolved.
Show resolved Hide resolved
flexDirection="column"
minHeight={0}
padding="zero"
jamesfan marked this conversation as resolved.
Show resolved Hide resolved
maxHeight={`calc(100vh - ${
elemProps.margin ? space[elemProps.margin as CanvasSpaceKeys] || elemProps.margin : space.xl
} * 2)`}
Expand Down
9 changes: 8 additions & 1 deletion modules/react/popup/lib/PopupHeading.tsx
Expand Up @@ -15,7 +15,14 @@ export const PopupHeading = createSubcomponent('h2')({
elemPropsHook: usePopupHeading,
})<PopupHeadingProps>(({children, ...elemProps}, Element) => {
return (
<Card.Heading as={Element} marginBottom="s" {...elemProps}>
<Card.Heading
as={Element}
marginInlineStart="l"
marginInlineEnd="l"
marginTop="l"
marginBottom="zero"
{...elemProps}
>
{children}
</Card.Heading>
);
Expand Down
8 changes: 5 additions & 3 deletions modules/react/popup/stories/examples/Basic.tsx
Expand Up @@ -9,7 +9,7 @@ import {
useInitialFocus,
useReturnFocus,
} from '@workday/canvas-kit-react/popup';
import {HStack} from '@workday/canvas-kit-react/layout';
import {Box, HStack} from '@workday/canvas-kit-react/layout';

export const Basic = () => {
const model = usePopupModel();
Expand All @@ -27,11 +27,13 @@ export const Basic = () => {
<Popup model={model}>
<Popup.Target as={DeleteButton}>Delete Item</Popup.Target>
<Popup.Popper placement="bottom">
<Popup.Card width={400} padding="s">
jamesfan marked this conversation as resolved.
Show resolved Hide resolved
<Popup.Card width={400}>
<Popup.CloseIcon aria-label="Close" />
<Popup.Heading>Delete Item</Popup.Heading>
<Popup.Body>
<p>Are you sure you'd like to delete the item titled 'My Item'?</p>
<Box as="p" marginTop="zero">
Are you sure you'd like to delete the item titled 'My Item'?
</Box>
<HStack spacing="s">
<Popup.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
Expand Down
22 changes: 12 additions & 10 deletions modules/react/popup/stories/examples/FocusRedirect.tsx
Expand Up @@ -10,7 +10,7 @@ import {
useFocusRedirect,
usePopupModel,
} from '@workday/canvas-kit-react/popup';
import {HStack} from '@workday/canvas-kit-react/layout';
import {Box, HStack} from '@workday/canvas-kit-react/layout';

export const FocusRedirect = () => {
const model = usePopupModel();
Expand Down Expand Up @@ -39,19 +39,21 @@ export const FocusRedirect = () => {
<Popup.Target as={DeleteButton}>Delete Item</Popup.Target>
<div aria-owns={popupId} style={{position: 'absolute'}} />
<Popup.Popper>
<Popup.Card width={400} padding="s">
<Popup.Card width={400}>
<Popup.CloseIcon aria-label="Close" />
<Popup.Heading>Delete Item</Popup.Heading>
<Popup.Body>
<p>Are you sure you'd like to delete the item titled 'My Item'?</p>
<Box as="p" marginTop="zero">
Are you sure you'd like to delete the item titled 'My Item'?
</Box>
<HStack spacing="s">
<Popup.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
</Popup.CloseButton>
{/* Disabled elements should not be focusable and focus should move to the next focusable element */}
<Popup.CloseButton disabled>Cancel</Popup.CloseButton>
</HStack>
</Popup.Body>
<HStack spacing="s">
<Popup.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
</Popup.CloseButton>
{/* Disabled elements should not be focusable and focus should move to the next focusable element */}
<Popup.CloseButton disabled>Cancel</Popup.CloseButton>
</HStack>
</Popup.Card>
</Popup.Popper>
<SecondaryButton>Next Focusable Button</SecondaryButton>
Expand Down
20 changes: 11 additions & 9 deletions modules/react/popup/stories/examples/FocusTrap.tsx
Expand Up @@ -10,7 +10,7 @@ import {
useReturnFocus,
usePopupModel,
} from '@workday/canvas-kit-react/popup';
import {HStack} from '@workday/canvas-kit-react/layout';
import {Box, HStack} from '@workday/canvas-kit-react/layout';

export const FocusTrap = () => {
const model = usePopupModel();
Expand Down Expand Up @@ -39,18 +39,20 @@ export const FocusTrap = () => {
<Popup.Target as={DeleteButton}>Delete Item</Popup.Target>
<div aria-owns={popupId} style={{position: 'absolute'}} />
<Popup.Popper>
<Popup.Card width={400} padding="s">
<Popup.Card width={400}>
<Popup.CloseIcon aria-label="Close" />
<Popup.Heading>Delete Item</Popup.Heading>
<Popup.Body>
<p>Are you sure you'd like to delete the item titled 'My Item'?</p>
<Box as="p" marginTop="zero">
Are you sure you'd like to delete the item titled 'My Item'?
</Box>
<HStack spacing="s">
<Popup.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
</Popup.CloseButton>
<Popup.CloseButton>Cancel</Popup.CloseButton>
</HStack>
</Popup.Body>
<HStack spacing="s">
<Popup.CloseButton as={DeleteButton} onClick={handleDelete}>
Delete
</Popup.CloseButton>
<Popup.CloseButton>Cancel</Popup.CloseButton>
</HStack>
</Popup.Card>
</Popup.Popper>
<SecondaryButton>Next Focusable Button</SecondaryButton>
Expand Down
18 changes: 10 additions & 8 deletions modules/react/popup/stories/examples/InitialFocus.tsx
Expand Up @@ -8,7 +8,7 @@ import {
useInitialFocus,
useReturnFocus,
} from '@workday/canvas-kit-react/popup';
import {HStack} from '@workday/canvas-kit-react/layout';
import {Box, HStack} from '@workday/canvas-kit-react/layout';

export const InitialFocus = () => {
const initialFocusRef = React.useRef(null);
Expand All @@ -25,17 +25,19 @@ export const InitialFocus = () => {
<Popup model={model}>
<Popup.Target>Send Message</Popup.Target>
<Popup.Popper placement={'bottom'}>
<Popup.Card width={400} padding="16px">
<Popup.Card width={400}>
<Popup.CloseIcon aria-label="Close" />
<Popup.Heading>Confirmation</Popup.Heading>
<Popup.Body>
<p id="popup-message">Your message has been sent!</p>
<Box as="p" marginTop="zero" id="popup-message">
Your message has been sent!
</Box>
<HStack spacing="s">
<Popup.CloseButton ref={initialFocusRef} aria-describedby="popup-message">
OK
</Popup.CloseButton>
</HStack>
</Popup.Body>
<HStack spacing="s">
<Popup.CloseButton ref={initialFocusRef} aria-describedby="popup-message">
OK
</Popup.CloseButton>
</HStack>
</Popup.Card>
</Popup.Popper>
</Popup>
Expand Down
34 changes: 17 additions & 17 deletions modules/react/popup/stories/examples/NestedPopups.tsx
Expand Up @@ -37,24 +37,24 @@ export const NestedPopups = () => {
<Popup.CloseIcon aria-label="Close" size="small" />
<Popup.Body>
<p>Contents of Popup 1</p>
<Popup model={popup2}>
<Popup.Target>Open Popup 2</Popup.Target>
<Popup.Popper>
<Popup.Card aria-label="Popup 2">
<Popup.CloseIcon aria-label="Close" size="small" />
<Popup.Body>
<p>Contents of Popup 2</p>
<HStack spacing="s">
<Popup.CloseButton as={Popup.CloseButton} model={popup1}>
Close Both (as)
</Popup.CloseButton>
<SecondaryButton {...closeBothProps}>Close Both (props)</SecondaryButton>
</HStack>
</Popup.Body>
</Popup.Card>
</Popup.Popper>
</Popup>
</Popup.Body>
<Popup model={popup2}>
<Popup.Target>Open Popup 2</Popup.Target>
<Popup.Popper>
<Popup.Card aria-label="Popup 2">
<Popup.CloseIcon aria-label="Close" size="small" />
<Popup.Body>
<p>Contents of Popup 2</p>
</Popup.Body>
<HStack spacing="s">
<Popup.CloseButton as={Popup.CloseButton} model={popup1}>
Close Both (as)
</Popup.CloseButton>
<SecondaryButton {...closeBothProps}>Close Both (props)</SecondaryButton>
</HStack>
</Popup.Card>
</Popup.Popper>
</Popup>
</Popup.Card>
</Popup.Popper>
</Popup>
Expand Down
16 changes: 9 additions & 7 deletions modules/react/popup/stories/examples/RTL.tsx
Expand Up @@ -3,21 +3,23 @@ import React from 'react';
import {SecondaryButton, DeleteButton} from '@workday/canvas-kit-react/button';
import {CanvasProvider, ContentDirection} from '@workday/canvas-kit-react/common';
import {Popup} from '@workday/canvas-kit-react/popup';
import {HStack} from '@workday/canvas-kit-react/layout';
import {Box, HStack} from '@workday/canvas-kit-react/layout';

export const RTL = () => {
return (
<CanvasProvider theme={{canvas: {direction: ContentDirection.RTL}}}>
<Popup.Card width={400} padding="s">
<Popup.Card width={400}>
<Popup.CloseIcon aria-label="סגור" />
<Popup.Heading>למחוק פריט</Popup.Heading>
<Popup.Body>
<p>האם ברצונך למחוק פריט זה</p>
<Box as="p" marginTop="zero">
האם ברצונך למחוק פריט זה
</Box>
<HStack spacing="s">
<DeleteButton>לִמְחוֹק</DeleteButton>
<SecondaryButton>לְבַטֵל</SecondaryButton>
</HStack>
</Popup.Body>
<HStack spacing="s">
<DeleteButton>לִמְחוֹק</DeleteButton>
<SecondaryButton>לְבַטֵל</SecondaryButton>
</HStack>
</Popup.Card>
</CanvasProvider>
);
Expand Down
3 changes: 1 addition & 2 deletions modules/react/toast/lib/Toast.tsx
Expand Up @@ -92,15 +92,14 @@ export const Toast = createComponent('div')({
ref={ref}
as={Element}
width={toastWidth}
padding="s"
depth={5}
role={isInteractive ? 'dialog' : isError ? 'alert' : 'status'}
aria-live={isInteractive ? 'off' : isError ? 'assertive' : 'polite'}
aria-atomic={!isInteractive}
{...elemProps}
>
{onClose && <Popup.CloseIcon aria-label="Close" onClick={onClose} size="small" />}
<Popup.Body>
<Popup.Body padding="s">
<ToastContentContainer onClose={onClose}>
{icon && <ToastSystemIcon color={iconColor} colorHover={iconColor} icon={icon} />}
<Message>
Expand Down