-
-
Notifications
You must be signed in to change notification settings - Fork 620
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
Update DefaultPage to be more general and add refresh button in web UI #233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
=======================================
Coverage 90.87% 90.87%
=======================================
Files 43 43
Lines 1677 1677
=======================================
Hits 1524 1524
Misses 82 82
Partials 71 71 Continue to review full report at Codecov.
|
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.
Thanks for your second contribution!
Haven't tested it yet, but looks pretty good overall :D.
ui/src/message/MessagesStore.ts
Outdated
public refreshByApp = async (appId: number) => { | ||
const state = this.stateOf(appId); | ||
const result = await this.fetchMessages(appId, 0).then((resp) => resp.data); | ||
state.messages.replace(result.messages); |
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.
I think we should refresh all messages because when refreshing only one application the "All Messages"-Tab will not get updated resulting in inconsistent state.
So we can use this.clearAll()
to discard our state and this.loadMore()
to get the most recent messages.
ui/src/common/DefaultPage.tsx
Outdated
buttonTitle?: string; | ||
fButton?: VoidFunction; | ||
buttonDisabled?: boolean; | ||
rightControl?: object; |
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.
This can probably be a React.ReactNode
Co-Authored-By: Jannis Mattheis <contact@jmattheis.de>
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.
Works great, thank you!
I've updated
DefaultPage
component to be more general following @jmattheis suggestions (see here).Now the component accepts a new argument
rightControl
useful to specify a react node placed on the top right corner of the page content.Old specific purpose arguments
buttonTitle
,fButton
,buttonDisabled
,hideButton
andbuttonId
were removed.Using the improved
DefaultPage
, we can easely solve the issue #171.