juliancwirko / react-s-alert

Alerts / Notifications for React with rich configuration options
https://www.npmjs.com/package/react-s-alert
MIT License
649 stars 69 forks source link

React 16 support #49

Closed martin-svk closed 7 years ago

martin-svk commented 7 years ago

react-s-alert@1.3.0 requires a peer of react@^15.0.0 but none was installed

An update to package.json deps should do it.

juliancwirko commented 7 years ago

Do you use React 16 ? I guess I need to make some updates for React 16

edit. :D oh sorry, I haven't seen the title ;) I'll update the package

stefensuhat commented 7 years ago

+1

juliancwirko commented 7 years ago

there is 1.3.1 version published, let me know if it helps, thanks

stefensuhat commented 7 years ago

1.3.1 fix react version. 👍

got one more

warning "react-s-alert@1.3.1" has incorrect peer dependency "prop-types@^15.5.4".

juliancwirko commented 7 years ago

hmmm, strange. I don't see it on clean app created with create-react-app. Can you test it on clean app?

stefensuhat commented 7 years ago

odd. After I've change my prop-type: ^15.6.0 the warning is gone. lol.

Thanks. no more warning from react-s-alert

jayprado commented 7 years ago

When I use this with React 16, my app crashes and I get the following error: TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.

juliancwirko commented 7 years ago

Interesting, can you test it with fresh, simple React 16 app? Just to eliminate other factors.

jayprado commented 7 years ago

@juliancwirko Just tested out a bare minimum instance with create-react-app. It gives out the same error. Here are the package versions:

dependencies: {
  "react": "^16.0.0",
  "react-dom": "^16.0.0",
  "react-s-alert": "^1.3.1",
  "react-scripts": "1.0.14"
}

and the line where the stack trace indicates the error: TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.

(anonymous function)
/s-alert-test/node_modules/react-s-alert/dist/s-alert-parts/s-alert-data-prep.js:135
135 | sAlertBoxHeight = parseInt(getComputedStyle(_reactDom2.default.findDOMNode(reactComponent))['height']);
juliancwirko commented 7 years ago

ok, thanks. Could you also post your configuration and how you use it?

jayprado commented 7 years ago

@juliancwirko Here's how I used it:

import React, { Component } from 'react';

import Alert from 'react-s-alert';
import 'react-s-alert/dist/s-alert-default.css';

class App extends Component {
  componentDidMount() {
    Alert.error("Test message")
  }

  render() {
    return (
      <div className="App">
        <Alert stack={{limit: 3}} />
      </div>
    );
  }
}

export default App;

Also, for some reason, it works when I wrap the Alert method call with setTimeout, like so:

componentDidMount() {
  setTimeout(() => Alert.error("Test message"), 0)
}

Let me know if you need anything else. Thanks a lot!

juliancwirko commented 7 years ago

Thanks, I need some time to test it. I'll comment here later.

lmeliberty commented 7 years ago

@jayarprado Thanks for the temporary solution.

anxobotana commented 7 years ago

@juliancwirko A nested requestAnimationFrame inside addToStore functions solves the issue.

Something like this:

const addToStoreTop = () => {
    requestAnimationFrame( () => requestAnimationFrame( () => {
            let length;
            storeStateTop = getAlertData('full-top') || [];
            length = storeStateTop.length;
            if (this.props.stack && this.props.stack.limit && length > this.props.stack.limit) {
                let id = storeStateTop[0].id;
                sAlertStore.dispatch({type: 'REMOVE', data: {id: id}});
                storeStateTop = getAlertData('full-top') || [];
            }
            this.setState({dataTop: storeStateTop});
    } ) );
};

The idea comes from: https://stanko.github.io/react-rerender-in-component-did-mount/

juliancwirko commented 7 years ago

I hope I'll be able to work on it on this weekend. At least I'll try to provide fix for this bug. Thanks.

juliancwirko commented 7 years ago

@jayarprado @anxobotana I still can't reproduce that ;) I copied this code:

import React, { Component } from 'react';

import Alert from 'react-s-alert';
import 'react-s-alert/dist/s-alert-default.css';

class App extends Component {
  componentDidMount() {
    Alert.error("Test message")
  }

  render() {
    return (
      <div className="App">
        <Alert stack={{limit: 3}} />
      </div>
    );
  }
}

export default App;

To my newly created app (totally fresh app created using create-react-app) and it works as it should. I don't get any errors even when using animation effect ;) weird...

I'll try to reproduce it anyway. Could you guys prepare reproduction repo for that? Then we will be sure that we have the same code. Thanks!

My browser: Chrome 59, Deps: "react": "^16.0.0", "react-dom": "^16.0.0", "react-scripts": "1.0.14", newest react-s-alert

juliancwirko commented 7 years ago

...ok nevermind ;) problem with npm link

juliancwirko commented 7 years ago

...should be fixed in 1.3.2

p1zzadog commented 7 years ago

works for me now, thank you!

juliancwirko commented 7 years ago

I'll close it for now. Feel free to comment here if needed.

chrisparton1991 commented 7 years ago

I can also confirm this is fixed, great work!

juliancwirko commented 7 years ago

cool :) also thanks to @anxobotana :+1:

binhnnph03565 commented 6 years ago

hi ! i setTimeOut for it <SAlert effect="stackslide" offset={-10} stack={{ limit: 1 }} timeout={6000} /> which it does not receive :(