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

[Collapsible] Fix collapsible transition behaviour #4000

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

spencercanner
Copy link
Contributor

@spencercanner spencercanner commented Feb 12, 2021

WHY are these changes introduced?

  • Fix a bug where transitions of elements inside the collapsible would trigger the component's transition end event logic (codesandbox example)

collapsible-bug

  • In the example above, using the button outside of the collapsible opens and closes it as expected. However, if a toggle button inside the collapsible is clicked, its transition is prematurely triggering the onTransitionEnd logic which fully closes the collapsible

WHAT is this pull request doing?

  • Add a check to the onTransitionEnd event that ensures it's coming from the container target, and not an element within it
    collapsible-fixed
  • This check used to exist before Collapsible was converted to a functional component

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React, {useCallback, useState} from 'react';

import {Button, Card, Collapsible, Page, Stack, TextContainer} from '../src';

export function Playground() {
  const [open, setOpen] = useState(true);

  const handleToggle = useCallback(() => {
    setOpen((open) => !open);
  }, []);

  return (
    <Page title="Playground">
      <div style={{height: '200px'}}>
        <Card sectioned>
          <Stack vertical>
            <Button
              onClick={handleToggle}
              ariaExpanded={open}
              ariaControls="basic-collapsible"
            >
              Toggle
            </Button>
            <Collapsible
              open={open}
              id="basic-collapsible"
              transition={{duration: '1000ms', timingFunction: 'ease-in-out'}}
            >
              <TextContainer>
                <p>
                  Your mailing list lets you contact customers or visitors who
                  have shown an interest in your store. Reach out to them with
                  exclusive offers or updates about your products.
                </p>
                <Button onClick={handleToggle}>Test button</Button>
              </TextContainer>
            </Collapsible>
          </Stack>
        </Card>
      </div>
    </Page>
  );
}

🎩 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
Copy link
Contributor

github-actions bot commented Feb 12, 2021

🟡 This pull request modifies 3 files and might impact 5 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: 5)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 5)

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

Files potentially affected (total: 0)

@spencercanner spencercanner force-pushed the fix-collapsible-transition branch 3 times, most recently from 692009a to 4f679f3 Compare February 16, 2021 21:34
@spencercanner spencercanner marked this pull request as draft February 17, 2021 21:24
@spencercanner spencercanner force-pushed the fix-collapsible-transition branch 2 times, most recently from ae4ada7 to 418e0c7 Compare February 17, 2021 22:58
@spencercanner spencercanner marked this pull request as ready for review February 17, 2021 23:01
@spencercanner spencercanner force-pushed the fix-collapsible-transition branch 2 times, most recently from f6fd622 to 483b08a Compare February 18, 2021 14:37
@spencercanner
Copy link
Contributor Author

spencercanner commented Feb 18, 2021

Percy is flagging a visual regression in the loading component but I can't trace this change to any updates in that component, so everything should be okay.

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Thanks for catching this @spencercanner. The dangers of re-writing a component from scratch without full test coverage. Nice work on the tests too ❤️

I left a comment around not setting props directly in tests but it looks like we do that in quite a few other places so I'll leave that up to you.

it('adds an isFullyClosed class to the collapsible onTransitionEnd if the event target is the collapsible div', () => {
const id = 'test-collapsible';
const collapsible = mountWithApp(<Collapsible id={id} open />);
collapsible.setProps({open: false});
Copy link
Member

Choose a reason for hiding this comment

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

My opinion is that we should avoid setting props directly. You can create a composed component with a button and trigger on click to change the state internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!
created a separate component in the test file to handle the prop updates by triggering a button

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👍 Thanks again!

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.

2 participants