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

Add modal with web support and animations #1698

Closed
wants to merge 4 commits into from
Closed

Add modal with web support and animations #1698

wants to merge 4 commits into from

Conversation

RichardLindhout
Copy link
Contributor

@RichardLindhout RichardLindhout commented Aug 11, 2020

Fixes: #1020

@RichardLindhout
Copy link
Contributor Author

Improvement would be listening to animationEnding instead of the

 setTimeout(() => {
        setDelayUnmounting(false)
      }, animationDurationMs + 100)

Also the internalVisible could be removed in favor of css animations instead of transition. Is it possible to add a css animation with React Native Web styles?

@RichardLindhout
Copy link
Contributor Author

I'm rewriting this partly to support multiple modals on top of each other while keeping focus at the current modal :)

@necolas
Copy link
Owner

necolas commented Aug 12, 2020

There's already another PR for modals that you should coordinate efforts with. I'd rather not use class components or Animated in the implementation. But that PR is more comprehensive. #1646 @imnotjames

@imnotjames
Copy link
Contributor

imnotjames commented Aug 12, 2020

👋 Hi there! Do check out the PR I'd opened a while back. It might be able to handle many of the use cases a modal should, along with a number of accessibility efforts baked in.

@necolas Oh, wonderful. You've moved away from using class components. At the time when I originally wrote the PR everything was using class components - so I kept it uniform with all the rest. I'll quickly update my PR to be uniform with the rest of the project!

@RichardLindhout
Copy link
Contributor Author

Ah that looks more complete indeed. I will post my final code here anyway since maybe there are things we could pick from this PR (when I update the code)

The animations would be nice to be done in CSS instead of Animated since Animated can be quite laggy with animating modals.

E.g.

 fadeIn: {
    animationKeyframes: [{ '0%': { opacity: 0 }, '100%': { opacity: 1 } }],
    animationDelay: 0,
    animationDuration: `${animationDurationMs}ms`,
    animationTimingFunction: 'linear',
  },
  slideIn: {
    animationKeyframes: [
      {
        '0%': {
          transform: [
            {
              translateY: '100%' as any,
            },
          ],
        },
        '100%': {
          transform: [
            {
              translateY: 0,
            },
          ],
        },
      },
    ],
    animationDelay: 0,
    animationDuration: `${animationDurationMs}ms`,
    animationTimingFunction: 'linear',
  },

ezgif-1-e9558d34d6f7

@imnotjames
Copy link
Contributor

I think the issue with the CSS animations was that I was having trouble setting up a callback to fire after they'd completed.

Is that possible with CSS animations now?

@RichardLindhout
Copy link
Contributor Author

RichardLindhout commented Aug 12, 2020

I did have the same problem, I just fixed it but it would be even cleaner if we could use the onTransitionEnd provided by React.

I now listen to 'transitionend' events.

@RichardLindhout
Copy link
Contributor Author

RichardLindhout commented Aug 12, 2020

Here is an updated example with the transition end.

I use the animation properties for the start animation and the transition properties for the out animation. I delay the modal closing till the transition out animation has ended.

ezgif-1-741df82248b4

@RichardLindhout
Copy link
Contributor Author

When a modal closes it also focused back on the previous modal.

@RichardLindhout
Copy link
Contributor Author

RichardLindhout commented Aug 13, 2020

[ x ] Hides app content from screen-readers when active.
[ x ] Traps focus.
[ x ] Built-in Escape-to-close mechanism.
[ x ] Focused on previous opened modal when modal on top of it closes
[ x ] Hides modals below visible ones for screen-readers

I think the other PR is more completed now (#1646). And is nicely done with animationEnd, looking good @imnotjames 👍

@necolas
Copy link
Owner

necolas commented Aug 18, 2020

Thanks for putting this together and helping with the other PR @RichardLindhout! Going to close this now and direct any further suggestions to the other PR under review

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: implementation
3 participants