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.

Sign up for GitHub

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

Jump to bottom

cockpit-components-logs-panel: React-related fixes #9343

Merged
merged 1 commit into from
Jul 24, 2018
Merged

cockpit-components-logs-panel: React-related fixes #9343

merged 1 commit into from
Jul 24, 2018

Conversation

mareklibra
Copy link
Contributor

Part of #9263 PR-split, see individual commits for more info.

@mareklibra mareklibra mentioned this pull request Jun 8, 2018
Merged
44 tasks
@martinpitt
Copy link
Member

Looks ok to me (aside from inconsistent quoting, but this is not important), but this needs a rebase against current master. Thanks!

@mareklibra mareklibra force-pushed the react.cockpit-components-logs-panel branch from 1fa23f9 to 3e9afea Compare June 11, 2018 06:45
@mareklibra
Copy link
Contributor Author

Rebased. Inconsistent quoting fixed.

martinpitt
martinpitt approved these changes Jun 11, 2018
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks!

martinpitt
martinpitt requested changes Jun 12, 2018
View reviewed changes
pkg/lib/cockpit-components-logs-panel.jsx Outdated
return (
<div className="cockpit-logline" role="row">
<div className="cockpit-logline" role="row" key={key}>
Copy link
Member

Choose a reason for hiding this comment

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

As we've found out in other PRs, using objects as key apparently doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

entry["__MONOTONIC_TIMESTAMP"] will work

mvollmer
mvollmer reviewed Jun 12, 2018
View reviewed changes
pkg/lib/cockpit-components-logs-panel.jsx Outdated
@@ -71,12 +73,12 @@ class JournalOutput {
}

render_day_header(day) {
return <div className="panel-heading">{day}</div>;
return <div className="panel-heading" key="day-header">{day}</div>;
Copy link
Member

Choose a reason for hiding this comment

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

There might be more than one day header.

mvollmer
mvollmer reviewed Jun 12, 2018
View reviewed changes
pkg/lib/cockpit-components-logs-panel.jsx Outdated
}

render_reboot_separator() {
return (
<div className="cockpit-logline" role="row">
<div className="cockpit-logline" role="row" key="reboot-sep">
Copy link
Member

Choose a reason for hiding this comment

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

There might be more than one reboot separator

mvollmer
mvollmer reviewed Jun 12, 2018
View reviewed changes
pkg/lib/cockpit-components-logs-panel.jsx
@@ -118,10 +120,13 @@ export class LogsPanel extends React.Component {
});
}

componentDillUnmount() {
componentWillUnmount() {
Copy link
Member

Choose a reason for hiding this comment

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

Ouch!

@mvollmer
Copy link
Member

I pushed some proposed fixes.

mareklibra
mareklibra commented Jun 15, 2018
View reviewed changes
Copy link
Contributor Author

@mareklibra mareklibra left a comment

Choose a reason for hiding this comment

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

LGTM

@mvollmer mvollmer force-pushed the react.cockpit-components-logs-panel branch from 2328331 to 09e1283 Compare June 22, 2018 11:12
@mvollmer
Copy link
Member

I squashed the commits and force pushed.

@mvollmer mvollmer requested a review from martinpitt July 6, 2018 12:35
martinpitt
martinpitt approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanups! LGTM.

@martinpitt
Copy link
Member

For some strange reason this causes ovirt build to fail:

chunk    {1} ovirt/vnc.min.js, ovirt/vnc.min.js.map (ovirt/vnc) 344 kB [rendered]

ERROR in ./pkg/ovirt/ovirt.less
Module build failed: 

      &:hover {
        background: @navbar-pf-item-hover-bg-color;
                  ^
Variable @navbar-pf-item-hover-bg-color is undefined
      in /tmp/source/node_modules/patternfly-react/dist/less/masthead.less (line 13, column 20)
 @ multi ovirt/ovirt
Child extract-text-webpack-plugin:

@mareklibra @martinpitt
cockpit-components-logs-panel: Fix typo and add keys
1b0e8bd 
Typo in React callback fixed.
React array element keys added.

A TODO for ongoing refactoring is added.

Closes cockpit-project#9343
@martinpitt martinpitt force-pushed the react.cockpit-components-logs-panel branch from 09e1283 to 1b0e8bd Compare July 24, 2018 09:00
@martinpitt
Copy link
Member

I pushed a rebase, and the mysterious ovirt failure in semaphore is gone now (semaphore doesn't rebase, but our integration tests do). So this is good now, waiting for tests.

@martinpitt martinpitt merged commit 75913a8 into cockpit-project:master Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mvollmer mvollmer mvollmer left review comments

@martinpitt martinpitt martinpitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mareklibra @martinpitt @mvollmer