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

feat: introduce alerts component to show alerts as alert-bars in a stack #486

Merged
merged 2 commits into from
Nov 16, 2020

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Nov 16, 2020

This is the component that goes with the alerts-service. It simply displays the alerts from the alert context as AlertBars in an AlertStack. The onHidden callback on the AlertBar will execute the onHidden passed as an option to useAlert and also the remove function on the alert, which will remove the alert from the context. onHidden is called from the AlertBar after the hide-animation completes, so things are hiding smoothly.

I'll leave some small notes in the code as well.

@@ -15,6 +16,7 @@ const App = ({ url, apiVersion, appName, children }) => (
<HeaderBar appName={appName} />
<AuthBoundary url={url}>
<div className="app-shell-app">{children}</div>
<Alerts />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt that inserting Alerts here, as a child of the AuthBoundary and sibling of div.app-shell-app was most appropriate. But that did force me to change prop-types on the AuthBoundary.

"@dhis2/d2-i18n": "^1.0.5",
"@dhis2/ui": "^5.4.2",
"@dhis2/ui": "^5.7.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We needed a higher version of @dhis2/ui which includes dhis2/ui#300 to avoid an infinite loop when showing more than one alert.

@@ -28,6 +28,6 @@ export const AuthBoundary = ({ url, children }) => {
}

AuthBoundary.propTypes = {
children: PropTypes.element,
children: PropTypes.node,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amcgee
Copy link
Member

amcgee commented Nov 16, 2020

I felt that inserting Alerts here, as a child of the AuthBoundary and sibling of div.app-shell-app was most appropriate. But that did force me to change prop-types on the AuthBoundary.

Hmmm... should we put it outside the Auth boundary? That way we can show alerts when, i.e., an authentication attempt fails.

@amcgee
Copy link
Member

amcgee commented Nov 16, 2020

Looks great! I think my only recommendation wouuld be to move the AlertStack outside the AuthBoundary

@HendrikThePendric HendrikThePendric merged commit fd22504 into master Nov 16, 2020
@HendrikThePendric HendrikThePendric deleted the feat/alerts branch November 16, 2020 15:29
dhis2-bot added a commit that referenced this pull request Nov 16, 2020
# [5.5.0](v5.4.2...v5.5.0) (2020-11-16)

### Bug Fixes

* **alerts:** make Alerts a sibling of AuthBoundary ([2b607b8](2b607b8))

### Features

* introduce alerts component to show alerts as alert-bars in a stack ([9aea659](9aea659))
* introduce alerts component to show alerts as alert-bars in a stack ([#486](#486)) ([fd22504](fd22504))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 5.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants