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

New Panels component #2001

Merged
merged 146 commits into from
Nov 19, 2024
Merged

New Panels component #2001

merged 146 commits into from
Nov 19, 2024

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Apr 15, 2024

Changes

Fixes #653. Introduces a new Panels API based partly on a coding session with @mayank99 (#2001 (comment)). This API allows users to easily create a multi panel UI with panel transition animations.

Basic usage code
const panelIdRoot = 'root';
const panelIdMoreInfo = 'more-info';

return (
  <Panels.Wrapper
    as={Surface}
    style={{
      inlineSize: 'min(300px, 30vw)',
      blockSize: 'min(500px, 50vh)',
    }}
  >
    <Panels.Panel id={panelIdRoot} as={Surface} border={false} elevation={0}>
      <Surface.Header as={Panels.Header}>Root</Surface.Header>
      <Surface.Body as={List}>
        <ListItem>
          <Panels.Trigger for={panelIdMoreInfo}>
            <ListItem.Action>More details</ListItem.Action>
          </Panels.Trigger>
        </ListItem>
      </Surface.Body>
    </Panels.Panel>

    <Panels.Panel
      id={panelIdMoreInfo}
      as={Surface}
      border={false}
      elevation={0}
    >
      <Surface.Header as={Panels.Header}>More details</Surface.Header>
      <Surface.Body isPadded>
        <Text>Content</Text>
      </Surface.Body>
    </Panels.Panel>
  </Panels.Wrapper>
);

Notes for reviewers:

  • Since this is a new component, all names are bike-sheddable.
  • I kept everything except the docs in this PR since I felt they are all related. But I can also split this PR, if needed.

Subcomponents:

  • Panels.Wrapper wraps all the panels. An explicit size must be given to Panels.Wrapper. The first Panels.Panel is is initial active panel.
  • Panels.Panel takes an id and the panel content. Match this id with a Panels.Triggers's for prop to create a link between them.
  • Panels.Trigger wraps the clickable element and appends an onClick to change the active panel to the one specified in the for prop. Also appends some attributes for accessibility. This component also creates a triggers map that stores a link between a <trigger + its current panel> and the panel it points to. This is useful for back navigation in Panels.Header.
  • Panels.BackButton (not exposed), goes to the previous panel (i.e. panel that has a trigger that points to the current panel).
  • Panels.Header is a required component to add an accessible name and also a back button (if previous panel exists) to the panel.

Usage requirements:

  • The initial displayed Panel should be the first Panel in the Panels.Wrapper.
  • A panel can have only one trigger pointing to it. i.e. out of all the triggers across all panels, only one can point to a particular panel.
    • Why: Since we use a simple map instead of a complex navigation history stack. Since in the map we can create one <trigger ⇔ panel> mapping, only one trigger can exist.
  • The Panels.Panel within the wrapper should be in the order of the navigation. E.g.:
    <Panels.Wrapper>
      <Panels.Panel id={root} /> // Must come before moreDetails since it contains the trigger to moreDetails
      <Panels.Panel id={moreDetails}> // Must come after root since it is navigated to from root
    </Panels.Wrapper>
    • Why: Since we use scrollIntoView() for the panel sliding effect, we scroll in the same direction as in the DOM. Thus, DOM order should be equal to the navigation order.

Implementation: All inactive panels are unmounted. When a trigger is clicked, the new panel is mounted, and the old panel is made inert. Focus is moved to the new panel's header's title text if going forward or to the triggering trigger if going backwards.

For the actual slide effect to transition between panels, we use scrollIntoView()+ scroll snapping to prevent abrupt perceived scroll jumps when DOM elements are added or removed. The backward slide effect when going back to the prev panel does NOT work in Firefox. But @mayank99 mentioned that this is alright since:

  • It's likely a Firefox bug that they'll fix if we report it
  • It's only the back animation. Forward animation works in all browsers.
Firefox behavior
demo.mov

The old panel says in the DOM for 500ms (arbitrary number) (#2001 (comment)). A private useDelayed() hook that @mayank99 suggested is used to achieve this.

The user's clicks are given priority over animations. Thus, when going to a new panel before the previous panel has been removed from the DOM (i.e. clicking the triggers in quick succession), the animations are intentionally not done to give priority to the user's clicks. (#2001 (comment)).

@mayank99 seemed to prefer the scrollIntoView() over the css animations and transforms despite the Firefox back slide effect not working issue. This is because the scroll approach less complex and keeps the door open for future changes such as keeping all panels always mounted.

Accessibility considerations:

  • Scrolling is instance when prefers-reduced-motion !== no-preference.

Instance:

  • To expose instance methods to the user, I re-used the useSynchronizeInstance from expose show()/close() methods via Dialog.useInstance() #1983. As suggested by @mayank99, I moved the useSynchronizeInstance call in an intermediate component so that we can replace useSynchronizeInstance in the future (if needed) without changing the component code.

Testing

Added e2e, unit, and image tests.

Screen recordings of demos

Basic Story in Edge:

demo.mov

Basic Story VO + Edge:

demo.mov

Multi level list story with and without VO. All in Safari:

output.mov

Docs

Proper docs will come in a new PR in this PR chain. That way I can write the docs based on our design decisions from this PR.

After PR TODO

  • See if we need to wrap the setControlledState (and maybe also the other arguments) of useControlledState with a useLatestRef (#2001 (comment)).

@r100-stack r100-stack self-assigned this Apr 15, 2024
@r100-stack r100-stack linked an issue Apr 15, 2024 that may be closed by this pull request
@mayank99
Copy link
Contributor

Before we get too far down this rabbithole, I just want to remind/note that this needs to be a generic pattern that shouldn't be tied to DropdownMenu. There are valid cases for a "sliding" panel that can be triggered by any arbitrary button within an InformationPanel or SideNavigation (submenu) or an AppUI widget.

Some other important requirements:

  • It should likely be implemented as a disclosure pattern.
  • It should probably move focus into the new panel when "opened".
  • Since the "original" panel is off screen, it should be hidden from keyboard/AT users.
  • It should contain a back button, which "closes" the panel and moves focus back.
  • Maybe it should close on pressing Esc? Unsure.

It might be tricky to come up with the right abstraction for this, but that's part of the exploration work.

@mayank99
Copy link
Contributor

Sharing this example from Firefox as an inspiration. It's basically a multi-level popover (rather than a menu).

I like the subtle animation and I like that it moves focus (although it's questionable that the focus is so far down the second screen. I think it should be on the back button instead).

Screen.Recording.2024-04-25.at.3.56.12.PM.mov

@mayank99
Copy link
Contributor

During a pairing session, we experimented with a generic Panels component that handles all the "panel switching" logic, and can be used anywhere (whether that's Popover or InformationPanel or Menu or an AppUI widget). It uses a simple disclosure pattern and manages focus.

Code
import {
  Flex,
  IconButton,
  List,
  ListItem,
  Surface,
  Text,
  ToggleSwitch,
} from '@itwin/itwinui-react';
import * as React from 'react';
import { useAtom, useAtomValue, useSetAtom, atom } from 'jotai';
import { SvgChevronLeft } from '@itwin/itwinui-icons-react';
import { flushSync } from 'react-dom';

const App = () => {
  const basePanelId = React.useId();
  const qualityPanelId = React.useId();
  const repeatId = React.useId();

  // Note: Will not work, because BackButton currently relies on context.
  const { goBack } = Panels.useInstance();

  return (
    <Surface style={{ display: 'inline-block' }}>
      <Panels defaultActiveId={basePanelId}>
        <Panel id={basePanelId}>
          <List>
            <ListItem>
              <Flex>
                <label htmlFor={repeatId}>Repeat</label>
                <Flex.Spacer />
                <ToggleSwitch id={repeatId} />
              </Flex>
            </ListItem>
            <ListItem>
              <Panel.Trigger for={qualityPanelId}>
                <ListItem.Action>Quality</ListItem.Action>
              </Panel.Trigger>
            </ListItem>
            <ListItem>Speed</ListItem>
            <ListItem>Loop</ListItem>
          </List>
        </Panel>

        <Panel id={qualityPanelId}>
          <Surface.Header as={Panel.Header}>Quality</Surface.Header>
          <List>
            <ListItem>
              <ListItem.Action
                onClick={() => {
                  // setQuality('240p');
                  goBack();
                }}
              >
                240p
              </ListItem.Action>
            </ListItem>
            <ListItem>360p</ListItem>
            <ListItem>480p</ListItem>
            <ListItem>720p</ListItem>
            <ListItem>1080p</ListItem>
          </List>
        </Panel>
      </Panels>
    </Surface>
  );
};

// ----------------------------------------------------------------------------

const expandedIdAtom = atom<string | undefined>(undefined);
const triggersAtom = atom(
  new Map<string, { triggerId: string; panelId: string }>(),
);

const Panels = ({
  children,
  defaultActiveId,
}: React.PropsWithChildren<any>) => {
  const [expandedId, setExpandedId] = useAtom(expandedIdAtom);

  if (expandedId === undefined) {
    setExpandedId(defaultActiveId);
  }

  return <>{children}</>;
};

const Panel = ({ children, id, ...rest }: React.PropsWithChildren<any>) => {
  const [expandedId] = useAtom(expandedIdAtom);
  return (
    <PanelIdContext.Provider value={id}>
      <div id={id} hidden={id !== expandedId} {...rest}>
        {children}
      </div>
    </PanelIdContext.Provider>
  );
};

const PanelIdContext = React.createContext('');

Panel.Header = ({ children, ...props }: React.PropsWithChildren<any>) => {
  return (
    <Flex {...props}>
      <Panel.BackButton />
      <Text
        as='h2'
        tabIndex={-1}
        // TODO: Confirm that focus moves correctly to the Text after the next panel is opened.
        // When a keyboard user triggers the panel, they should be able to continue tabbing into the panel.
        // When a screen-reader user triggers the panel, they should hear the name of the panel announced.
        //
        // Alternate idea: maybe the Panel itself could be focused. But then the panel needs a role and a label.
        ref={React.useCallback((el: HTMLElement | null) => el?.focus(), [])}
      >
        {children}
      </Text>
    </Flex>
  );
};

Panel.BackButton = () => {
  const setExpandedId = useSetAtom(expandedIdAtom);
  const panelId = React.useContext(PanelIdContext);
  const trigger = useAtomValue(triggersAtom).get(panelId);

  const goBack = () => {
    flushSync(() => setExpandedId(trigger?.panelId));

    if (trigger?.triggerId) {
      document.getElementById(trigger?.triggerId)?.focus();
    }
  };

  return (
    <IconButton
      label='Back'
      styleType='borderless'
      onClick={goBack}
      size='small'
      data-iui-shift='left'
    >
      <SvgChevronLeft />
    </IconButton>
  );
};

Panels.useInstance = () => ({
  goBack: () => {},
});

Panel.Trigger = ({
  children: childrenProp,
  for: forProp,
}: React.PropsWithChildren<{ for: string }>) => {
  const [expandedId, setExpandedId] = useAtom(expandedIdAtom);
  const [triggers, setTriggers] = useAtom(triggersAtom);
  const panelId = React.useContext(PanelIdContext);

  const children = React.Children.only(childrenProp) as any;

  const triggerFallbackId = React.useId();
  const triggerId = children?.props?.id || triggerFallbackId;

  if (triggers.get(forProp)?.triggerId !== triggerId) {
    setTriggers(new Map(triggers.set(forProp, { triggerId, panelId })));
  }

  return (
    React.isValidElement(children) &&
    React.cloneElement(children, {
      id: triggerId,
      onClick: () => setExpandedId(forProp),
      'aria-expanded': expandedId === forProp,
      'aria-controls': forProp,
    } as any)
  );
};

export default App;

@r100-stack r100-stack changed the base branch from main to r/unstable-docs-starter November 18, 2024 13:50
packages/itwinui-css/src/panels/panels.scss Outdated Show resolved Hide resolved

return (
<Panels.Wrapper as={Surface} className='demo-panels-wrapper'>
<Panels.Panel id={panelIdRoot} as={Surface} border={false} elevation={0}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the examples, it doesn't feel right to use two Surfaces. What exactly from Surface do we need on Panels.Panel?

I remember you had tried Flex instead of Surface but it wasn't enough for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Panels.Panel needs Surface since it sometimes uses Surface.Header and Surface.Body inside it.

I put a Surface on Panels.Wrapper too so that we can get an outer border on the entire container. I then removed the border on each panel so that there will be no border between the panels when the panels are transitioning.


We could remove the Surface on the Panels.Wrapper if we have a Surface on the Panels.Panel where the border is false. The border: false is to avoid showing a border between two panels when transitioning. But the outer border will then not be there.

Surface on Wrapper; borderless Surface on Panel borderless Surface on Panel Surface on Panel
3.mov
2.mov
1.mov

Removing Surface from Panels.Panel instead of Panels.Wrapper works for the examples with some minor change of styles. But since Surface.Header and Surface.Body are no longer under Surface, in some cases, things may not work without custom styles.

But since the examples are working with this second option, I implemented it in e163abf0..acab7f63.


Flex used to not work in some cases. I think that was when we were using transforms for the transitions. But they now seem to work in the above mentioned commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave this comment open, in case we want to revisit this decision in the future (I'm sure some edge cases will come up).

Copy link
Member Author

@r100-stack r100-stack Nov 19, 2024

Choose a reason for hiding this comment

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

Just wanted to mention that looks like not having Surface would need us to add something like Flex to add the scrollbar when the Surface.Body overflows. E.g. some of the changes in 4a7abbc.

If Surface.Body was directly underneath Surface, the scrollbar on Surface.Body's overflow would be handled automatically.

apps/website/src/components/PropsTable.astro Outdated Show resolved Hide resolved
Base automatically changed from r/unstable-docs-starter to main November 18, 2024 21:59
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

:shipit:

@r100-stack r100-stack enabled auto-merge (squash) November 19, 2024 21:12
@r100-stack r100-stack merged commit b829983 into main Nov 19, 2024
18 checks passed
@r100-stack r100-stack deleted the rohan/layered-dropdown-menu branch November 19, 2024 21:14
@imodeljs-admin imodeljs-admin mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layered Drop Down Menu
3 participants