[Collapsible] functional component and no jank when opening #3606

alex-page · 2020-11-05T23:15:37Z

WHY are these changes introduced?

Fixes #3390

WHAT is this pull request doing?

Convert collapsible to a functional component
Fix janky movement when opening collapsible filters
Remove opacity from animation
Support nested collapsible elements

How to 🎩

Go to the collapsible example
Go to the filters example and open and close the filters

🎩 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
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2020-11-05T23:17:52Z

🟡 This pull request modifies 6 files and might impact 6 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 6)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

📄 scripts/build-validate.js (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Collapsible/Collapsible.scss (total: 6)

Files potentially affected (total: 6)

🧩 src/components/Collapsible/Collapsible.tsx (total: 5)

Files potentially affected (total: 5)

📄 src/components/Collapsible/README.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Collapsible/tests/Collapsible.test.tsx (total: 0)

Files potentially affected (total: 0)

BPScott · 2020-11-06T01:19:46Z

Yay hookification. Not reviewed this but could you update #1995 when it gets merged?

alex-page · 2020-11-13T16:27:41Z

src/components/Collapsible/Collapsible.tsx

+  useEffect(() => {
+    if (open === isOpen) {
      return;
    }

-    requestAnimationFrame(() => {
-      const heightNode = this.heightNode.current;
-      switch (animationState) {
-        case 'idle':
-          break;
-        case 'measuring':
-          this.setState({
-            animationState: wasOpen ? 'closingStart' : 'openingStart',
-            height: wasOpen && heightNode ? heightNode.scrollHeight : 0,
-          });
-          break;
-        case 'closingStart':
-          this.setState({
-            animationState: 'closing',
-            height: 0,
-          });
-          break;
-        case 'openingStart':
-          this.setState({
-            animationState: 'opening',
-            height: heightNode ? heightNode.scrollHeight : 0,
-          });
-      }
-    });
-  }
+    setHeight(collapisbleContainer.current.scrollHeight);
+  }, [open]);


This useEffect measured the component and applies the height. For open this is the only effect ran as it measures and applies, this causes the transition from 0 -> measured height. For closing we do the same thing but then use the second useEffect to transition to 0.

alex-page · 2020-11-13T16:28:29Z

src/components/Collapsible/Collapsible.tsx

-  if (animationState === 'idle' && open) {
-    return open ? 'none' : undefined;
-  }
+    getComputedStyle(collapisbleContainer.current).height;


This is needed otherwise the rendering engine measures and applies the height then set the new height causing no transition. This causes the rendering engine to apply the 0 height on the next cycle causing the transition to run.

alex-page · 2020-11-13T16:29:10Z

src/components/Collapsible/README.md

 }
 ```

+### Nested collapsible


Is there a way to exclude examples from the styleguide? This isn't best practice but is important that it works.

We'd need something like  but I don't think we have one at the moment

Would be great if we could grab context from the admin where nested collapsible are used to provide context on valid use cases. Unfortunately, I can't remember where it's being used, only that is it 😕

Add ../src/components/**/*.stories.tsx to https://github.com/Shopify/polaris-react/blob/master/.storybook/main.js#L18

Then create a Collapsible.stories.tsx and dump your storybook stories in there using the standard CSF format

alex-page · 2020-11-13T16:29:37Z

@AndrewMusgrave @kyledurand this should be good now.

kyledurand · 2020-11-13T21:03:28Z

Maybe the smallest regression. Not sure if it's a blocker https://screenshot.click/screencast_2020-11-13_16-02-48.mp4

kyledurand

Code looks good 👍

kyledurand · 2020-11-13T21:06:45Z

src/components/Collapsible/Collapsible.scss

+  overflow: hidden;
+  will-change: height;
+  transition-property: height;
+  transition-duration: duration(fast);


Do we want to use var(--p-duration-1-0-0) here instead?

Edit: nvm looks like we need to use tokens to match some JS below

kyledurand · 2020-11-13T21:13:25Z

src/components/Collapsible/README.md

 }
 ```

+### Nested collapsible


We'd need something like  but I don't think we have one at the moment

alex-page · 2020-11-13T21:17:11Z

Would we ever want it to finish the animation before closing? I think you would want it to respond instantly on click. I could be wrong though..

AndrewMusgrave

The animation was acting oddly when opening/closing the collapsible quickly. Sometimes it didn't do anything, other times the animation would jank. It's an edge case so, whatever you think is best. Keep in mind the animation is at 10% speed.

Left a note about rendering children. Some tests in web expect on that - not sure about their implementations though. Just something to keep in mind when releasing this.

AndrewMusgrave · 2020-11-20T16:28:57Z

src/components/Collapsible/README.md

 }
 ```

+### Nested collapsible


Would be great if we could grab context from the admin where nested collapsible are used to provide context on valid use cases. Unfortunately, I can't remember where it's being used, only that is it 😕

src/components/Collapsible/README.md

AndrewMusgrave · 2020-11-20T16:31:31Z

src/components/Collapsible/tests/Collapsible.test.tsx

-    expect(collapsible.contains('content')).toBe(false);
-  });
-
-  it('does not render its children when going from open to closed', () => {


Unfortunately, this is an important test. Some consumer implementations rely on this - I'm sure you're going to see some tests failing in web

Is it necessary to not render or tests will now fail?

Tests will probably fail (unless the code has been refactored 🤞 ) - it might be easier to release a rc and run CI with that branch. I don't think UI will break - but it would be worthwhile checking the failed tests.

I'd like to remind y'all about https://buildkite.com/shopify/polaris-react-system-integration-test which runs a pass of the web CI on arbitary polaris branches.

It runs by default on our master branch, but it'd be trivial to trigger a manual build that points at this branch.

No faffing with RCs needed if you just want to test unit tests.

src/components/Collapsible/Collapsible.scss

Co-authored-by: Andrew Musgrave <andrew.musgrave@shopify.com>

johanstromqvist · 2020-12-07T15:57:47Z

Looks great!! Thanks for the storyboard link @alex-page.

How is duration determined? If the value is fixed, a 500px collapse will be perceived as 10x the speed of a 50px collapse.

Dynamic containers benefit from dynamically determined durations. @AndrewMusgrave and I collaborated once to create a dynamic interpolation with normalized speed and clamped min/max values. Could that perhaps be applied?

alex-page · 2020-12-07T16:05:21Z

@johanstromqvist made a new issue for this #3682

johanstromqvist · 2020-12-07T16:33:11Z

The interruptibility seems wonky when the transition is triggered a second time before it has finished. Only on collapse.

alex-page self-assigned this Nov 5, 2020

alex-page force-pushed the collapsible-fix branch from 9fef20b to d42b27e Compare November 5, 2020 23:17

alex-page requested review from BPScott and kyledurand November 5, 2020 23:17

alex-page requested a review from johanstromqvist November 5, 2020 23:19

alex-page force-pushed the collapsible-fix branch 2 times, most recently from 681e249 to 378b54c Compare November 5, 2020 23:29

alex-page mentioned this pull request Nov 6, 2020

Post v4 cleanup work #1995

Closed

alex-page requested a review from AndrewMusgrave November 6, 2020 02:08

alex-page changed the title ~~Collapsible functional component and no jank~~ [Collapsible] functional component and no jank when opening Nov 6, 2020

alex-page force-pushed the collapsible-fix branch from 5aab10e to 3538e5d Compare November 6, 2020 02:54

alex-page requested a review from emmaorhun November 6, 2020 18:34

alex-page force-pushed the collapsible-fix branch from 0a5bed9 to 35c04d4 Compare November 13, 2020 16:21

alex-page commented Nov 13, 2020

View reviewed changes

alex-page requested a review from AndrewMusgrave November 13, 2020 16:29

alex-page force-pushed the collapsible-fix branch from 6236a7c to c01feb7 Compare November 13, 2020 19:50

Shopify deleted a comment from kyledurand Nov 13, 2020

Shopify deleted a comment from AndrewMusgrave Nov 13, 2020

functional component and no jank when opening

ea47fa2

alex-page force-pushed the collapsible-fix branch from c01feb7 to ea47fa2 Compare November 13, 2020 21:09

kyledurand approved these changes Nov 13, 2020

View reviewed changes

AndrewMusgrave approved these changes Nov 20, 2020

View reviewed changes

kyledurand mentioned this pull request Nov 27, 2020

[Navigation] Add aria attributes for sub nav items #3661

Merged

6 tasks

Update src/components/Collapsible/README.md

9011858

Co-authored-by: Andrew Musgrave <andrew.musgrave@shopify.com>

alex-page mentioned this pull request Dec 7, 2020

Collapsible duration ratio based on content size #3682

Closed

Alex Page added 4 commits December 9, 2020 09:58

Merge branch 'master' into collapsible-fix

51c94e8

Merge branch 'master' into collapsible-fix

f09dc8f

Merge branch 'master' into collapsible-fix

5f88d24

Working collapsible, needs display none when closed for a11y

4e0c287

alex-page force-pushed the master branch from ee1f920 to 405548f Compare December 21, 2020 18:28

Merge branch 'master' into collapsible-fix

5162c7c

alex-page closed this Jan 7, 2021

alex-page deleted the collapsible-fix branch January 7, 2021 17:36

[Collapsible] functional component and no jank when opening #3606

[Collapsible] functional component and no jank when opening #3606

Uh oh!

Conversation

alex-page commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

github-actions bot commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 6)

Files potentially affected (total: 5)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Uh oh!

BPScott commented Nov 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-page commented Nov 13, 2020

Uh oh!

kyledurand commented Nov 13, 2020

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-page commented Nov 13, 2020

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BPScott Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johanstromqvist commented Dec 7, 2020

Uh oh!

alex-page commented Dec 7, 2020

Uh oh!

johanstromqvist commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alex-page commented Nov 5, 2020 •

edited

Loading

github-actions bot commented Nov 5, 2020 •

edited

Loading

BPScott Dec 15, 2020 •

edited

Loading

johanstromqvist commented Dec 7, 2020 •

edited

Loading