search

puranjayjain / react-materialui-notifications

Spec compliant notifications for react and material ui users
https://puranjayjain.github.io/react-materialui-notifications/
MIT License
251 stars 36 forks source link

Enhancement: Optimize the hierarchy #132

Open wittyPuneet opened 7 years ago

wittyPuneet commented 7 years ago

I am working on my first project using the wonderful material-ui library, and your notifications library. As part of my project, I needed to make the notifications be available globally. I pulled up your component and connected it with Redux. However, I noticed that none of my leave animations were working.

Digging through the source code, I noticed that there is a separate ReactCSSTransitionGroup being created for each notification, and the keys for all the child elements were same (perhaps the side effect of pulling the component up in a wrapper component).

Further digging revealed that the hierarchy of the various elements is as follows: ReactMaterialUINotification>Notification>ReactCSSTransitionGroup>List>List Item

It appears that there is separate ReactCSSTransitionGroup and List being created for each notification.

Wouldn't it be better to have it as such: ReactMaterialUINotification>List>ReactCSSTransitionGroup>List Item (as Notification) ?

puranjayjain commented 7 years ago

that is quite insightful, @wittyPuneet could you start a Pull Request regarding this? It will be really helpful for fixing this issue.

wittyPuneet commented 7 years ago

Refactoring the hierarchy is going to require rewriting all the styles. I am still a novice, and with material-ui its a nightmare with all the inline styles. Hence, its going to take a bit of time. I will try to refactor the hierarchy once I have some free time from my current project.

puranjayjain commented 7 years ago

If that's the case It would wonderful if both are efforts are focused towards #64