-
Notifications
You must be signed in to change notification settings - Fork 264
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
Till/2400 message banner #2421
Till/2400 message banner #2421
Changes from all commits
d5e769f
77cb0ad
839132e
13258f4
4b72483
bc1a5b7
100e858
d82f02d
a6827e5
4e3410d
82a85c3
3f2c80c
224b1ea
6f7e8ad
552f312
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import axios from 'axios'; | ||
import asyncActionCreator from 'actions/asyncActionCreator'; | ||
|
||
export const fetchMessages = asyncActionCreator( | ||
(endpoint) => async () => { | ||
const response = await axios.get(endpoint); | ||
return { messages: response.data }; | ||
}, | ||
{ name: 'FETCH_MESSAGES' } | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,15 @@ import { Route, Navigate, Routes } from 'react-router-dom'; | |
import { connect } from 'react-redux'; | ||
import { Spinner } from '@blueprintjs/core'; | ||
|
||
import { fetchMetadata } from 'actions'; | ||
import { selectSession, selectMetadata } from 'selectors'; | ||
import { fetchMetadata, fetchMessages } from 'actions'; | ||
import { | ||
selectSession, | ||
selectMetadata, | ||
selectMessages, | ||
selectPinnedMessage, | ||
} from 'selectors'; | ||
import Navbar from 'components/Navbar/Navbar'; | ||
import MessageBanner from 'components/MessageBanner/MessageBanner'; | ||
|
||
import NotFoundScreen from 'screens/NotFoundScreen/NotFoundScreen'; | ||
import OAuthScreen from 'screens/OAuthScreen/OAuthScreen'; | ||
|
@@ -34,24 +40,55 @@ import ExportsScreen from 'src/screens/ExportsScreen/ExportsScreen'; | |
|
||
import './Router.scss'; | ||
|
||
const MESSAGES_INTERVAL = 15 * 60 * 1000; // every 15 minutes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why a check every 15 minutes? What are the implications of a shorter/longer interval? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refreshing the messages more frequently seemed a little wasteful. The worst case scenario here is that users will see an update 15 minutes after it has been published, which seemed acceptable to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something I noticed when I was playing with the message banner earlier in the week was that updates to the UI were inconsistent. I reduced the refresh to 60 seconds and even accounting for the build process is was really hit and miss as to when the update would show. I also couldn't figure out how long the "fixed" green banner would persist? |
||
|
||
class Router extends Component { | ||
componentDidMount() { | ||
this.fetchIfNeeded(); | ||
this.setMessagesInterval(); | ||
} | ||
|
||
componentDidUpdate() { | ||
this.fetchIfNeeded(); | ||
} | ||
|
||
componentWillUnmount() { | ||
this.clearMessagesInterval(); | ||
} | ||
|
||
fetchIfNeeded() { | ||
const { metadata } = this.props; | ||
const { metadata, messages } = this.props; | ||
|
||
if (metadata.shouldLoad) { | ||
this.props.fetchMetadata(); | ||
} | ||
|
||
if (messages.shouldLoad) { | ||
this.fetchMessages(); | ||
} | ||
} | ||
|
||
fetchMessages() { | ||
const { metadata } = this.props; | ||
|
||
if (metadata?.app?.messages_url) { | ||
this.props.fetchMessages(metadata.app.messages_url); | ||
} | ||
} | ||
|
||
setMessagesInterval() { | ||
const id = setInterval(() => this.fetchMessages(), MESSAGES_INTERVAL); | ||
this.setState(() => ({ messagesInterval: id })); | ||
} | ||
|
||
clearMessagesInterval() { | ||
if (this.state?.messagesInterval) { | ||
clearInterval(this.state.messagesInterval); | ||
} | ||
} | ||
|
||
render() { | ||
const { metadata, session } = this.props; | ||
const { metadata, session, pinnedMessage } = this.props; | ||
const isLoaded = metadata && metadata.app && session; | ||
|
||
const Loading = ( | ||
|
@@ -61,13 +98,15 @@ class Router extends Component { | |
</div> | ||
</div> | ||
); | ||
|
||
if (!isLoaded) { | ||
return Loading; | ||
} | ||
|
||
return ( | ||
<> | ||
<Navbar /> | ||
<MessageBanner message={pinnedMessage} /> | ||
<Suspense fallback={Loading}> | ||
<Routes> | ||
<Route path="oauth" element={<OAuthScreen />} /> | ||
|
@@ -160,7 +199,12 @@ class Router extends Component { | |
|
||
const mapStateToProps = (state) => ({ | ||
metadata: selectMetadata(state), | ||
messages: selectMessages(state), | ||
pinnedMessage: selectPinnedMessage(state), | ||
session: selectSession(state), | ||
}); | ||
|
||
export default connect(mapStateToProps, { fetchMetadata })(Router); | ||
export default connect(mapStateToProps, { | ||
fetchMetadata, | ||
fetchMessages, | ||
})(Router); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import { Callout, Intent, Classes } from '@blueprintjs/core'; | ||
import { injectIntl } from 'react-intl'; | ||
import { RelativeTime } from 'components/common'; | ||
|
||
import './MessageBanner.scss'; | ||
|
||
const MESSAGE_INTENTS = { | ||
info: Intent.PRIMARY, | ||
warning: Intent.WARNING, | ||
error: Intent.DANGER, | ||
success: Intent.SUCCESS, | ||
}; | ||
|
||
function Wrapper({ children }) { | ||
return ( | ||
<div className="MessageBanner" role="status" aria-atomic="true"> | ||
{children} | ||
</div> | ||
); | ||
} | ||
|
||
function MessageBanner({ message }) { | ||
if (!message) { | ||
return <Wrapper />; | ||
} | ||
|
||
const intent = MESSAGE_INTENTS[message.level] || Intent.WARNING; | ||
const updates = message.updates || []; | ||
|
||
const latestUpdate = | ||
updates.length > 0 ? updates[updates.length - 1] : message; | ||
|
||
return ( | ||
<Wrapper> | ||
<Callout intent={intent} icon={null} className="MessageBanner__callout"> | ||
<p> | ||
{message.title && ( | ||
<> | ||
<strong className={Classes.HEADING}>{message.title}</strong> | ||
<br /> | ||
</> | ||
)} | ||
|
||
<span | ||
dangerouslySetInnerHTML={{ __html: latestUpdate.safeHtmlBody }} | ||
/> | ||
|
||
{latestUpdate.createdAt && ( | ||
<span className="MessageBanner__meta"> | ||
<RelativeTime date={latestUpdate.createdAt} /> | ||
</span> | ||
)} | ||
</p> | ||
</Callout> | ||
</Wrapper> | ||
); | ||
} | ||
|
||
export default injectIntl(MessageBanner); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
@import '../../app/variables.scss'; | ||
|
||
.MessageBanner__callout.MessageBanner__callout { | ||
padding: 0.5 * $aleph-content-padding $aleph-content-padding; | ||
} | ||
|
||
.MessageBanner p { | ||
margin: 0; | ||
} | ||
|
||
.MessageBanner a, | ||
.MessageBanner a:hover { | ||
color: inherit; | ||
text-decoration: underline; | ||
font-weight: 600; | ||
} | ||
|
||
.MessageBanner__meta { | ||
opacity: 0.6; | ||
} | ||
|
||
.MessageBanner__meta::before { | ||
content: ' — '; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we prefer? URI or URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URL is fine, it seems consistent with the settings portion of the line above, which isn't actually consistent with itself, hmmm.