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: Revert displaying popovers as modals on mobile for now #7392

Merged
merged 6 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions packages/@react-spectrum/s2/src/FullscreenDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export const dialogInner = style({
'header',
'.',
'content',
'.',
'buttons'
],
sm: [
Expand All @@ -90,6 +91,7 @@ export const dialogInner = style({
'auto',
24,
'1fr',
24,
'auto'
],
sm: [
Expand Down
53 changes: 26 additions & 27 deletions packages/@react-spectrum/s2/src/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,19 @@ import {
composeRenderProps,
Dialog,
DialogProps,
ModalRenderProps,
OverlayArrow,
OverlayTriggerStateContext,
useLocale
} from 'react-aria-components';
import {colorScheme, getAllowedOverrides, StyleProps, UnsafeStyles} from './style-utils' with {type: 'macro'};
import {ColorSchemeContext} from './Provider';
import {DismissButton} from 'react-aria';
import {DOMRef} from '@react-types/shared';
import {forwardRef, MutableRefObject, useCallback, useContext} from 'react';
import {keyframes} from '../style/style-macro' with {type: 'macro'};
import {mergeStyles} from '../style/runtime';
import {Modal} from './Modal';
import {style} from '../style' with {type: 'macro'};
import {StyleString} from '../style/types' with {type: 'macro'};
import {useDOMRef, useIsMobileDevice} from '@react-spectrum/utils';
import {useDOMRef} from '@react-spectrum/utils';

export interface PopoverProps extends UnsafeStyles, Omit<AriaPopoverProps, 'arrowSize' | 'isNonModal' | 'arrowBoundaryOffset' | 'isKeyboardDismissDisabled' | 'shouldCloseOnInteractOutside' | 'shouldUpdatePosition'> {
Copy link
Member

Choose a reason for hiding this comment

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

should we get rid of the mobileType prop for now if we're not showing popovers as modals on mobile?

styles?: StyleString,
Expand All @@ -44,9 +41,9 @@ export interface PopoverProps extends UnsafeStyles, Omit<AriaPopoverProps, 'arro
/**
* The size of the Popover. If not specified, the popover fits its contents.
*/
size?: 'S' | 'M' | 'L',
size?: 'S' | 'M' | 'L'
/** The type of overlay that should be rendered when on a mobile device. */
mobileType?: 'modal' | 'fullscreen' | 'fullscreenTakeover' // TODO: add tray back in
// mobileType?: 'modal' | 'fullscreen' | 'fullscreenTakeover' // TODO: add tray back in
}

const fadeKeyframes = keyframes(`
Expand Down Expand Up @@ -126,6 +123,9 @@ let popover = style({
L: 576
}
},
// Don't be larger than full screen minus 2 * containerPadding
maxWidth: '[calc(100vw - 24px)]',
Copy link
Member

Choose a reason for hiding this comment

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

If there are scrollbars showing, this will still position and look okay, right?

boxSizing: 'border-box',
translateY: {
placement: {
bottom: {
Expand Down Expand Up @@ -221,9 +221,7 @@ function PopoverBase(props: PopoverProps, ref: DOMRef<HTMLDivElement>) {
UNSAFE_className = '',
UNSAFE_style,
styles,
size,
children,
trigger = null
size
} = props;
let domRef = useDOMRef(ref);
let colorScheme = useContext(ColorSchemeContext);
Expand All @@ -239,24 +237,25 @@ function PopoverBase(props: PopoverProps, ref: DOMRef<HTMLDivElement>) {
}, [locale, direction, domRef]);

// On small devices, show a modal (or eventually a tray) instead of a popover.
let isMobile = useIsMobileDevice();
if (isMobile && process.env.NODE_ENV !== 'test') {
let mappedChildren = typeof children === 'function'
? (renderProps: ModalRenderProps) => children({...renderProps, defaultChildren: null, trigger, placement: 'bottom'})
: children;
// TODO: reverted this until we have trays.
// let isMobile = useIsMobileDevice();
// if (isMobile && process.env.NODE_ENV !== 'test') {
// let mappedChildren = typeof children === 'function'
// ? (renderProps: ModalRenderProps) => children({...renderProps, defaultChildren: null, trigger, placement: 'bottom'})
// : children;

return (
<Modal size={size} isDismissable>
{composeRenderProps(mappedChildren, (children, {state}) => (
<>
{children}
{/* Add additional dismiss button at the end to match popovers. */}
<DismissButton onDismiss={state.close} />
</>
))}
</Modal>
);
}
// return (
// <Modal size={size} isDismissable>
// {composeRenderProps(mappedChildren, (children, {state}) => (
// <>
// {children}
// {/* Add additional dismiss button at the end to match popovers. */}
// <DismissButton onDismiss={state.close} />
// </>
// ))}
// </Modal>
// );
// }

// TODO: this still isn't the final popover 'tip', copying various ones out of the designs files yields different results
// containerPadding not working as expected
Expand Down Expand Up @@ -289,7 +288,7 @@ function PopoverBase(props: PopoverProps, ref: DOMRef<HTMLDivElement>) {
let _PopoverBase = forwardRef(PopoverBase);
export {_PopoverBase as PopoverBase};

export interface PopoverDialogProps extends Pick<PopoverProps, 'size' | 'hideArrow' | 'mobileType' | 'placement' | 'shouldFlip' | 'containerPadding' | 'offset' | 'crossOffset'>, Omit<DialogProps, 'className' | 'style'>, StyleProps {}
export interface PopoverDialogProps extends Pick<PopoverProps, 'size' | 'hideArrow'| 'placement' | 'shouldFlip' | 'containerPadding' | 'offset' | 'crossOffset'>, Omit<DialogProps, 'className' | 'style'>, StyleProps {}

const dialogStyle = style({
padding: 8,
Expand Down
8 changes: 6 additions & 2 deletions packages/@react-spectrum/s2/stories/Popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* governing permissions and limitations under the License.
*/

import {ActionButton, Avatar, Button, Card, CardPreview, Content, DialogTrigger, Divider, DropZone, Form, Image, Menu, MenuItem, MenuSection, Popover, SearchField, SubmenuTrigger, Switch, Tab, TabList, TabPanel, Tabs, Text, TextField} from '../src';
import {ActionButton, Avatar, Button, Card, CardPreview, Content, DialogTrigger, Divider, Form, Image, Menu, MenuItem, MenuSection, Popover, SearchField, SubmenuTrigger, Switch, Tab, TabList, TabPanel, Tabs, Text, TextField} from '../src';
import Cloud from '../s2wf-icons/S2_Icon_Cloud_20_N.svg';
import Education from '../s2wf-icons/S2_Icon_Education_20_N.svg';
import File from '../s2wf-icons/S2_Icon_File_20_N.svg';
Expand Down Expand Up @@ -97,7 +97,6 @@ export const HelpCenter = (args: any) => (
<Form>
<TextField label="Subject" />
<TextField label="Description" isRequired />
<DropZone />
<Switch>Adobe can contact me for further questions concerning this feedback</Switch>
<Button styles={style({marginStart: 'auto'})}>Submit</Button>
</Form>
Expand Down Expand Up @@ -159,3 +158,8 @@ export const AccountMenu = (args: any) => (
</Popover>
</DialogTrigger>
);

AccountMenu.argTypes = {
hideArrow: {table: {disable: true}},
placement: {table: {disable: true}}
};