[Collapsible] Add support for disabling the transition, and fix state machine #7364

Bringer128 · 2022-10-06T17:54:54Z

WHY are these changes introduced?

Today the Collapsible component can get stuck in the "animating" state for 2 reasons, both of which cause the onTransitionEnd event not to fire.

The prop transition={{ duration: "0ms" }} is used as Navigation Items does.
The children of the Collapsible have no margin and it is rendered for the first time with open=true (as raised in the linked issue). This causes the div to have max-height not change, which means the transition does not occur.

Fundamentally the issue with this component is that it relies on the onTransitionEnd event to move out of the animating state. There are 2 ways that this event won't fire, similarly related:

If the animation is started but the height doesn't change (e.g. from 0px to 0px)
If the animation is started but the duration is set to 0 (0ms, 0s or -some-var-that-evaluates-to-0)

This PR does not solve the dependency on the onTransitionEnd event. Instead it:

Adds formal support for disabling the transition by setting transition={false}. This prevents the need to pass 0ms duration. (Note this does not stop the bug occurring)
Handles if the user passes 0ms or 0s to also disable the transition.
Removes the "re-measure on child update" logic, as this was a workaround for the fact that the component was getting stuck in the animating state.

As such there will still be a bug if the computed height of children is set to 0px. However I'm choosing not to address this in this PR as the chances are rare. To trigger this bug you must:

Render the Collapsible in closed state (open={false})
Toggle the open state to true, while the children is 0px high as computed by element.scrollHeight
Change the children to have content

WHAT is this pull request doing?

It reverts the changes from [Collapsible] Optionally prevent measuring onChildUpdate. #6283 and [Collapsible] Re-measure when children change #6100 which caused the linked bug.
Adds support for passing false to the transition prop to disable the transition.
Changes the Navigation Secondary component to use transition={false}

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import {HomeMinor, OrdersMinor, ProductsMinor} from '@shopify/polaris-icons';
import React, {useState} from 'react';

import {Button, Card, Collapsible, Frame, Navigation} from '../src';

const list1 = [
  {label: 'a', url: 'path/to/one'},
  {label: 'b', url: 'path/to/two'},
];
const list2 = [
  {label: 'one', url: 'path/to/one'},
  {label: 'two', url: 'path/to/two'},
  {label: 'three', url: 'path/to/three'},
];
const list3 = [
  {label: '1', url: 'path/to/one'},
  {label: '2', url: 'path/to/two'},
  {label: '3', url: 'path/to/three'},
  {label: '4', url: 'path/to/four'},
  {label: '5', url: 'path/to/five'},
];

export function Playground() {
  const [open, setOpen] = useState(true);
  const toggleOpen = () => {
    setOpen(!open);
  };
  const scenario1 = (
    <div>
      <h1>Scenario 1</h1>
      <Collapsible open={open} id="1">
        <ul>
          <li>List item</li>
        </ul>
      </Collapsible>
    </div>
  );

  const scenario2 = (
    <div>
      <h1>Scenario 2</h1>
      <button onClick={toggleOpen}>Toggle open</button>
      <Collapsible open={open} id="2">
        <div>Hello</div>
      </Collapsible>
    </div>
  );

  const [selected, setSelected] = useState('home');
  const [subitems, setSubitems] = useState(list2);

  const scenario3 = (
    <Frame>
      <button onClick={() => setSubitems(list1)}>list 1</button>
      <button onClick={() => setSubitems(list2)}>list 2</button>
      <button onClick={() => setSubitems(list3)}>list 3</button>
      <Navigation location="/">
        <Navigation.Section
          items={[
            {
              url: '/',
              label: 'Home',
              icon: HomeMinor,
              selected: selected === 'home',
              onClick: () => setSelected('home'),
            },
            {
              url: '/',
              label: 'Orders',
              icon: OrdersMinor,
              subNavigationItems: subitems,
              selected: selected === 'orders',
              onClick: () => setSelected('orders'),
            },
            {
              url: '/',
              label: 'Products',
              icon: ProductsMinor,
              selected: selected === 'products',
              onClick: () => setSelected('products'),
            },
          ]}
        />
        <Navigation.Item
          url="/"
          label="Expected List"
          subNavigationItems={subitems}
        />
      </Navigation>
    </Frame>
  );
  const scenario4 = (
    <Card>
      Scenario 4
      <Collapsible id="4" open={open} transition={{duration: '100ms'}}>
        Content goes here
      </Collapsible>
    </Card>
  );

  const [firstOpen, setFirstOpen] = useState(false);
  const [secondOpen, setSecondOpen] = useState(false);
  const scenario5 = (
    <Card>
      <h1>Scenario 5</h1>
      <button onClick={() => setFirstOpen(!firstOpen)}>First</button>
      <button onClick={() => setSecondOpen(!secondOpen)}>Second</button>
      <Collapsible id="5" open={firstOpen} transition={{duration: '100ms'}}>
        <Collapsible id="5.1" open={secondOpen}>
          <div
            style={{width: '100px', height: '100px', backgroundColor: 'purple'}}
          />
        </Collapsible>
      </Collapsible>
    </Card>
  );

  const scenario6 = (
    <Card title="Scenario 6">
      <Collapsible id="6" open={open}>
        <div style={{height: 0}} />
      </Collapsible>
    </Card>
  );
  return (
    <div>
      <button onClick={toggleOpen}>Toggle open</button>
      {scenario1}
      {scenario2}
      {scenario3}
      {scenario4}
      {scenario5}
      {scenario6}
    </div>
  );
}

The code has 6 scenarios:

A Collapsible with ul as its child (this has margin-top and margin-bottom)
A Collapsible with div as its child (this has no margins)
The original scenario from [Collapsible] Re-measure when children change #6100 to show that this does not cause a regression.
Duration 100ms explicitly set
Nested collapsibles
A Collapsible that has children that is 0px high (not fixed by this PR)

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

…ldren change.

github-actions · 2022-10-06T17:59:30Z

size-limit report 📦

Path	Size
polaris-react-cjs	209.11 KB (+0.03% 🔺)
polaris-react-esm	135.59 KB (+0.06% 🔺)
polaris-react-esnext	190.95 KB (+0.04% 🔺)
polaris-react-css	41.52 KB (0%)

… minor bump

kyledurand

This is looking good for 0ms | 0s durations but there's a wee update loop bug when a non-zero duration is passed in.

collapsible.mp4

I can give you a hand tomorrow or Monday if you want it

Once we get rid of the loop we should:

Make sure this works with nested Collapsible components
Make sure this covers the needs of the last two contributors
/snapit and ping folks for 🎩

kyledurand · 2022-10-06T18:25:09Z

.changeset/light-houses-repeat.md

+
+Deprecated Collapsible preventMeasuringOnChildrenUpdate.
+Fixed bug where Collapsible would get stuck in animating state.
+Support for 0ms transition duration in Collapsible.


This also fixes #4194 👏

polaris-react/src/components/Collapsible/Collapsible.tsx

aishad · 2022-10-06T20:43:47Z

/snapit

github-actions · 2022-10-06T20:49:56Z

🫰✨ Thanks @aishad! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221006204649

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221006204649

yarn add @shopify/polaris@0.0.0-snapshot-release-20221006204649

Bringer128 · 2022-10-07T13:57:44Z

This is looking good for 0ms | 0s durations but there's a wee update loop bug when a non-zero duration is passed in.

I can give you a hand tomorrow or Monday if you want it

Once we get rid of the loop we should:

Make sure this works with nested Collapsible components

Make sure this covers the needs of the last two contributors

/snapit and ping folks for 🎩

Thanks for finding this! This was caused by adding a new dependency to the useEffect that triggers the transition. Every time the height changed, the startAnimation callback would change, which would trigger the useEffect to move to measuring state again. This caused the max-height to flip-flop between renders as it was trying to restart the animation. The incoming commit fixes it.

I also found thanks to your comment that 100ms was being caught by my regex because I forgot to catch the start and end of line stuff.

I notice in the Storybook for Collapsible that it sets the duration to a CSS variable. This was a scenario I hadn't considered, and the bug will still be present if the CSS variable evaluates to 0s or 0ms. Perhaps we should make disable an explicit prop? It looks like there are kind of hacky JavaScript ways to extract the global styles from the document element but it probably makes more sense to make an explicit prop.

…hanges.

aishad · 2022-10-07T15:04:05Z

I 🎩 to make sure there was no regression for our use case, and all looks good.
Created https://github.com/Shopify/web/issues/75074 to remove the prop from our Component.

Bringer128 · 2022-10-07T15:06:59Z

This blog shows how to get the value from a CSS property: https://www.designcise.com/web/tutorial/how-to-get-the-value-of-a-css-variable-using-javascript

In theory in this component we could do the following code:

const duration = transition?.duration;
const disableTransition = useMemo(() => {
  if(!duration || !collapsibleContainer.current) return false;
  if(/^0+(ms|s)$/.test(duration.trim()) return true;

  const styles = getComputedStyle(collapsibleContainer.current);
  const computedDuration = styles.getPropertyValue(duration);
  return /^0+(ms|s)$/.test(computedDuration);
}, [duration]);

kyledurand · 2022-10-07T15:51:50Z

I'd prefer a prop over reading computed styles. I left this thought in slack but I'll leave it here as well for others to chime in but what if we merged the transitionDisabled prop with transition. The interface would look something like:

/** Assign transition properties to the collapsible
 * @default true
 */
transition?: boolean | Transition;

<Collapsible transition="false" /> 
<Collapsible transition={{duration: 'var(--p-duration-150)', timingFunction: 'var(--p-ease-in-out)'}} />

kyledurand · 2022-10-07T16:00:32Z

/snapit

github-actions · 2022-10-07T16:04:35Z

🫰✨ Thanks @kyledurand! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221007160227

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221007160227

yarn add @shopify/polaris@0.0.0-snapshot-release-20221007160227

Bringer128 · 2022-10-07T20:09:45Z

/snapit

Bringer128 · 2022-10-07T20:30:03Z

/snapit

github-actions · 2022-10-07T20:35:33Z

🫰✨ Thanks @Bringer128! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221007203242

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221007203242

yarn add @shopify/polaris@0.0.0-snapshot-release-20221007203242

…sabled

Bringer128 · 2022-10-11T17:57:57Z

/snapit

Bringer128 · 2022-10-11T17:58:44Z

@kyledurand I've updated the PR description based on the solution we chose; and would love your fresh 👀 . I've tested nested Collapsibles and a few browsers and it all seems to work fine.

github-actions · 2022-10-11T18:03:08Z

🫰✨ Thanks @Bringer128! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221011180026

yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221011180026

yarn add @shopify/polaris@0.0.0-snapshot-release-20221011180026

kyledurand

This is looking really good. My main concern would be disabling transition when 0 values are passed in as duration. Adding that means consumers don't have to use the new false flag for disabling the transition

kyledurand · 2022-10-12T15:05:58Z

polaris-react/src/components/Collapsible/Collapsible.tsx

-  transition?: Transition;
-  /** Prevents component from re-measuring when child is updated **/
+  /** Override transition properties. When set to false, disables transition completely. Defaults to true, which uses
+   * default transition properties.


Suggested change

* default transition properties.

* @default transition={{duration: 'var(--p-duration-150)', timingFunction: 'var(--p-ease-in-out)'}}

polaris-react/src/components/Collapsible/Collapsible.tsx

kyledurand · 2022-10-12T15:32:03Z

polaris-react/src/components/Collapsible/Collapsible.tsx

@@ -51,11 +53,14 @@ export function Collapsible({
    expandOnPrint && styles.expandOnPrint,
  );

+  const transitionDisabled = transition === false;


I think we still care about transition.duration being 0 || 0ms || 0s here. Can we disable transition on those values as well?

As discussed in Slack, the CSS spec requires the unit for the transition-duration field so I'm not addressing the 0 scenario.

polaris-react/src/components/Collapsible/tests/Collapsible.test.tsx

kyledurand

👏 Fantastic work @Bringer128. Thanks for taking this on 🥇

@aaronccasanova

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris-migrator@0.5.0 ### Minor Changes - [#7373](#7373) [`56c82ee8d`](56c82ee) Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Add `getFunctionArgs` utility ### Patch Changes - Updated dependencies \[[`c3f427c17`](c3f427c)]: - @shopify/polaris-tokens@6.2.1 ## @shopify/polaris@10.8.0 ### Minor Changes - [#7364](#7364) [`e4b2c36d8`](e4b2c36) Thanks [@Bringer128](https://github.com/Bringer128)! - Deprecated Collapsible preventMeasuringOnChildrenUpdate. Fixed bug where Collapsible would get stuck in animating state when duration is 0. Add support for intentionally disabling the transition in Collapsible. ### Patch Changes - [#7363](#7363) [`8a6c323e2`](8a6c323) Thanks [@aveline](https://github.com/aveline)! - Added `id` prop to `Text` and `Box` - [#7348](#7348) [`ea2a45bbb`](ea2a45b) Thanks [@aveline](https://github.com/aveline)! - Added `setMediaWidth` breakpoints test utility - [#7388](#7388) [`5bc885765`](5bc8857) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed a re-render bug with Page Actions - Updated dependencies \[[`c3f427c17`](c3f427c)]: - @shopify/polaris-tokens@6.2.1 ## @shopify/plugin-polaris@0.0.11 ### Patch Changes - Updated dependencies \[[`56c82ee8d`](56c82ee)]: - @shopify/polaris-migrator@0.5.0 ## @shopify/polaris-tokens@6.2.1 ### Patch Changes - [#7385](#7385) [`c3f427c17`](c3f427c) Thanks [@laurkim](https://github.com/laurkim)! - Refactored exported alias and scale types in `breakpoints`, `depth`, `font`, `motion`, `shape`, `spacing`, and `zIndex`. ## @shopify/stylelint-polaris@4.3.2 ### Patch Changes - Updated dependencies \[[`c3f427c17`](c3f427c)]: - @shopify/polaris-tokens@6.2.1 ## polaris.shopify.com@0.22.0 ### Minor Changes - [#7032](#7032) [`40ee692aa`](40ee692) Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - Added Playroom integration to Polaris docs site. ### Patch Changes - [#7032](#7032) [`40ee692aa`](40ee692) Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - Improved the design of the Sandbox feature. - [#7400](#7400) [`9f9fe1f99`](9f9fe1f) Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed a scaling bug caused by content overflow Fixed a bug where examples that don't have any content wouldn't show up - Updated dependencies \[[`8a6c323e2`](8a6c323), [`e4b2c36d8`](e4b2c36), [`c3f427c17`](c3f427c), [`ea2a45bbb`](ea2a45b), [`5bc885765`](5bc8857)]: - @shopify/polaris@10.8.0 - @shopify/polaris-tokens@6.2.1 ## polaris-for-figma@0.0.24 ### Patch Changes - Updated dependencies \[[`8a6c323e2`](8a6c323), [`e4b2c36d8`](e4b2c36), [`ea2a45bbb`](ea2a45b), [`5bc885765`](5bc8857)]: - @shopify/polaris@10.8.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Bringer128 added 2 commits October 6, 2022 13:34

Do not animate when transition duration is 0. Do not animate when chi…

fe4e2a0

…ldren change.

Collapsible no longer needs preventMeasuringOnChildrenUpdate

cdbcf0a

Bringer128 requested a review from kyledurand October 6, 2022 17:58

Bringer128 added 3 commits October 6, 2022 14:05

Don't remove the preventMeasuringOnChildrenUpdate flag to make this a…

c06133c

… minor bump

Deprecate the preventMeasuringOnChildrenUpdate property

f08519a

Add changeset

258bdf9

Bringer128 requested a review from acmertz October 6, 2022 19:48

kyledurand reviewed Oct 6, 2022

View reviewed changes

Bringer128 added 2 commits October 7, 2022 10:09

Change to use disableTransition prop. Limit useEffect to open state c…

12ee027

…hanges.

Fix test

0a49421

Merge the transition properties

063b41f

Secondary to use merged transition

4b341c5

Bringer128 changed the title ~~[Collapsible] Handle transition duration of 0~~ [Collapsible] Add support for disabling the transition, and fix state machine Oct 11, 2022

Bringer128 added 4 commits October 11, 2022 10:56

Changeset updated to new changes

237ea07

Update tests to cover fully open, closed, animating and transition di…

20017ed

…sabled

Remove accidental comment

5a0528f

Fix test typo

82f3e5a

kyledurand reviewed Oct 12, 2022

View reviewed changes

acmertz reviewed Oct 12, 2022

View reviewed changes

polaris-react/src/components/Collapsible/tests/Collapsible.test.tsx Outdated Show resolved Hide resolved

Add support for catching 0ms and 0s as well

e14e13a

kyledurand approved these changes Oct 13, 2022

View reviewed changes

Bringer128 merged commit e4b2c36 into main Oct 13, 2022

Bringer128 deleted the collapsible_handle_no_transition branch October 13, 2022 16:16

github-actions bot mentioned this pull request Oct 13, 2022

Version Packages #7382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Collapsible] Add support for disabling the transition, and fix state machine #7364

[Collapsible] Add support for disabling the transition, and fix state machine #7364

Bringer128 commented Oct 6, 2022 •

edited

Loading

github-actions bot commented Oct 6, 2022 •

edited

Loading

kyledurand left a comment •

edited

Loading

kyledurand Oct 6, 2022

aishad commented Oct 6, 2022

github-actions bot commented Oct 6, 2022

Bringer128 commented Oct 7, 2022

aishad commented Oct 7, 2022

Bringer128 commented Oct 7, 2022

kyledurand commented Oct 7, 2022

kyledurand commented Oct 7, 2022

github-actions bot commented Oct 7, 2022

Bringer128 commented Oct 7, 2022

Bringer128 commented Oct 7, 2022

github-actions bot commented Oct 7, 2022

Bringer128 commented Oct 11, 2022

Bringer128 commented Oct 11, 2022

github-actions bot commented Oct 11, 2022

kyledurand left a comment

kyledurand Oct 12, 2022

kyledurand Oct 12, 2022

Bringer128 Oct 13, 2022

kyledurand left a comment

	* default transition properties.
	* @default transition={{duration: 'var(--p-duration-150)', timingFunction: 'var(--p-ease-in-out)'}}

[Collapsible] Add support for disabling the transition, and fix state machine #7364

[Collapsible] Add support for disabling the transition, and fix state machine #7364

Conversation

Bringer128 commented Oct 6, 2022 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

github-actions bot commented Oct 6, 2022 • edited Loading

size-limit report 📦

kyledurand left a comment • edited Loading

Choose a reason for hiding this comment

kyledurand Oct 6, 2022

Choose a reason for hiding this comment

aishad commented Oct 6, 2022

github-actions bot commented Oct 6, 2022

Bringer128 commented Oct 7, 2022

aishad commented Oct 7, 2022

Bringer128 commented Oct 7, 2022

kyledurand commented Oct 7, 2022

kyledurand commented Oct 7, 2022

github-actions bot commented Oct 7, 2022

Bringer128 commented Oct 7, 2022

Bringer128 commented Oct 7, 2022

github-actions bot commented Oct 7, 2022

Bringer128 commented Oct 11, 2022

Bringer128 commented Oct 11, 2022

github-actions bot commented Oct 11, 2022

kyledurand left a comment

Choose a reason for hiding this comment

kyledurand Oct 12, 2022

Choose a reason for hiding this comment

kyledurand Oct 12, 2022

Choose a reason for hiding this comment

Bringer128 Oct 13, 2022

Choose a reason for hiding this comment

kyledurand left a comment

Choose a reason for hiding this comment

Bringer128 commented Oct 6, 2022 •

edited

Loading

github-actions bot commented Oct 6, 2022 •

edited

Loading

kyledurand left a comment •

edited

Loading