schiehll / react-alert

alerts for React
MIT License
608 stars 98 forks source link

Fix for direct alert position property #115

Closed besLisbeth closed 5 years ago

besLisbeth commented 5 years ago

Hi Reinaldo! I have finally found an issue for resolving the bugs which I created:)

So, the main point is that I tried to add the node Wrapper between TransitionGroup and Transition and that was my main fault, despite the fact, that I redirected props to the needed elements. For such case,theTransitionGrouphave propertycomponent` and I refactored it the next way:

<TransitionGroup
     appear
     key={position}
     options={{ position, containerStyle }}
     component={Wrapper}
     {...props}
>
{...}
</TransitionGroup>

If we'll look at the source code of the react-transition-group we will see, that all extra-properties are redirected to component, that's why I added property options to the TransitionGroup itself:

TransitionGroup.js

render() {
    const { component: Component, childFactory, ...props } = this.props
    {....}

    if (Component === null) {
      return children
    }
    return <Component {...props}>{children}</Component>
  }
}

I also found out, that animation broke the react-select package because authors have bug there and open issue.

Because of my fixes I had a need to rewrite the test a little bit, so I hope that you'll have a look and tell me if you like it or not.

Waiting for the feedback!)

schiehll commented 5 years ago

Hey @besLisbeth sorry about the delay to review it, I was kinda busy.

So I've tested it locally and everything works fine as far as I can tell, great job!

But I'm not so convinced about the tests...it looks like it should be taking at least the amount of time defined in the transitionTime var, doesn't it? It doesn't seems to be the case, as the "react-alert with one Provider and minimum needed options" group of tests for example takes less than 200ms to run in my machine while the value defined in transitionTime is 250.

Something doesn't look right, can you please take a look on it? I guess a good point of start would be the jest timer docs.

Also, running yarn eslint __tests__ we got the following:

Screen Shot 2019-04-07 at 15 34 05

Can you please take a look on those too? Thank you!

besLisbeth commented 5 years ago

@schiehll, I fixed the tests. Really sorry for the first version( Please, give feedback when you have time. If something is still not right, I'll do my best to fix it!

schiehll commented 5 years ago

Everything is looking good now, just that little thing that I've commented in the review, great work!

besLisbeth commented 5 years ago

Hi @schiehll! I'm really sorry, but did you see that I've fixed the test?

schiehll commented 5 years ago

Hey @besLisbeth! I've been super busy last month, sorry for taking so long to get back to this.

I will try to take a look at it later today, promise.

besLisbeth commented 5 years ago

Everything is alright, take your time! I just was confused if you've seen it.

schiehll commented 5 years ago

Great work as always @besLisbeth! Thank you so much for your time here contributing with code and closing issues!

besLisbeth commented 5 years ago

Thank you a lot for letting me do it! Feel free to contact me if something goes wrong - hope, that it won't happen this time.