-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Pod logs refactoring #1516
Pod logs refactoring #1516
Conversation
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
this.isJumpButtonVisible = false; | ||
} | ||
if (!logs.length) { | ||
this.isLastLineVisible = false; |
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 field really sounds like it is computed. Shouldn't it be marked as such?
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 don't think so. This is an observable flag not really different from other ones like isJumpButtonVisible
. A computed
is used to calculate some value from other observables. isLastLineVisible
just holds bool value and doesn't need to be "computed".
reaction(() => this.props.tab.id, async () => { | ||
await this.load(); | ||
this.scrollToBottom(); | ||
await this.reload(); | ||
}, { fireImmediately: true }), |
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.
Couldn't we just pass this.reload
to the reaction directly?
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.
Can you elaborate this a bit please?
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.
Sure. It seems like we have the following snippet. async () => { await this.reload(); }
. I wonder if we could instead just have, this.reload
if it has been marked with @autobind()
.
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.
Yes, it works as expected. Thanks for review!
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Refactoring log components by spreading
PodLogs
monolith to logical and presentational ones.Also fixing founded bugs including: