Skip to content

Conversation

@mitchmaps
Copy link
Contributor

@mitchmaps mitchmaps commented Sep 4, 2019

Why are these changes introduced?

Fixes #1897

What issue is this pull request addressing?

The normal fade up animation present in the modal component currently doesn't appear when setting the component directly to true.

What is this pull request doing?

This PR addresses this by appending additional appear animation classes to the component so its consistent throughout. S/o to @dleroux for suggesting this solution in the issue 🙏

Click to view collapsed before and after gifs

Before

Jul-30-2019 14-08-21

After

Jul-30-2019 14-11-20

If you have any questions reach out to @ mitchell.soares on Slack!

@mitchmaps mitchmaps requested a review from loic-d September 4, 2019 15:32
Copy link
Member

@chloerice chloerice 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 taking this on, @mitchmaps!! 🎩 👀 👍Add a note to UNRELEASED.md && :shipit:

### Enhancements

### Bug fixes
- Fixed animation for Modal when being rendered asynchronously ([#2076](https://github.com/Shopify/polaris-react/pull/2076))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably just say 'when being mounted with open:true directly' or something like that?
Rendering the modal asynchronously using react-async is one of the scenario where it's mounted directly with open:true but there are others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, thanks!


const fadeUpClasses = {
appear: classNames(styles.animateFadeUp, styles.entering),
appearActive: classNames(styles.animateFadeUp, styles.entered),
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@loic-d loic-d left a comment

Choose a reason for hiding this comment

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

🎩 💯

I noticed that we have the same transition issue when closing the Modal using the open && <Modal open ... /> pattern.

import React, {useState} from 'react';
import {Page, Modal, Button} from '../src';

export function Playground() {
  const [active, setActive] = useState(false);
  const handleChange = () => {
    setActive(!active);
  };

  return (
    <Page title="Playground">
      <div style={{height: '500px'}}>
        <Button onClick={handleChange}>Open</Button>
        {active && (
          <Modal
            open
            onClose={handleChange}
            title="Reach more shoppers with Instagram product tags"
            primaryAction={{
              content: 'Add Instagram',
              onAction: handleChange,
            }}
            secondaryActions={[
              {
                content: 'Learn more',
                onAction: handleChange,
              },
            ]}
          >
            <Modal.Section>
              <p>
                Use Instagram posts to share your products with millions of
                people. Let shoppers buy from your store without leaving
                Instagram.
              </p>
            </Modal.Section>
          </Modal>
        )}
      </div>
    </Page>
  );
}

I would still 🚢 this PR in the meantime 👍

@mitchmaps
Copy link
Contributor Author

Talked to @dleroux about the lack of a matching exit animation brought up by @loic-d above. We agreed to just ship this one that fixes the opening animation error and open a second issue addressing the closing one. 🚢

@mitchmaps mitchmaps merged commit df56f15 into master Sep 4, 2019
@mitchmaps mitchmaps deleted the fix-modal-animation branch September 4, 2019 20:38
@ghost
Copy link

ghost commented Sep 4, 2019

🎉 Thanks for your contribution to Polaris React!

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.

[Modal] / Missing animation when component is rendered with open set to true directly

3 participants