apps: React-related fixes #9344

mareklibra · 2018-06-08T09:36:25Z

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

The DBus-related issue was observed in tests when working on #9263.

martinpitt · 2018-06-11T03:58:17Z

pkg/apps/application-list.jsx

-                            <td>{refresh_progress}</td>
-                            <td>{refresh_button}</td>
-                        </tr>
+                        <tbody>


Ugh, is a tbody really always required in React? That sounds strange, it's entirely optional in HTML and only really useful if one needs to shove in additional things next to the tr?

martinpitt · 2018-06-11T03:59:15Z

pkg/apps/application.jsx

@@ -80,11 +80,11 @@ class Application extends React.Component {

            return description.map(paragraph => {
                if (paragraph.tag == 'ul') {
-                    return <ul>{paragraph.items.map(item => <li>{item}</li>)}</ul>;
+                    return <ul key={paragraph}>{paragraph.items.map(item => <li key={item}>{item}</li>)}</ul>;


This would repeat the entire paragraph text in the HTML, which seems rather useless and wasteful? What is the purpose of all these keys?

Discussed in #9339 , the long paragraph text will not make it to the HTML

But paragraph is not a string...

Replacing it with string paragraph-${index}. Use of indexes is not preferred, but not anti-pattern and working.

mareklibra · 2018-06-11T07:02:31Z

It's a good practice to explicitly use the <tbody>. Discussed i.e. here: facebook/react#5652

For that reason, I'll need to fix a few CSS selectors within check-* scripts, since the DOM structure is changed. Anyway, it's already done within #9263 , so it will get into these "split" PRs when relevant.

martinpitt

Thanks! LGTM then. I marked it as blocked, AFAIUI the check-storage PR has to land first, right?

mareklibra · 2018-06-11T13:17:03Z

Files under pkg/apps are modified only in this PR, so I think the storage fixes are unrelated.
For storage, some of the fixes will be just similar to this one.

mvollmer · 2018-06-14T12:55:21Z

pkg/apps/utils.jsx

@@ -91,7 +91,7 @@ export const show_error = ex => {
    if (ex.code == "cancelled")
        return;

-    if (ex.code == "not-found")
+    if (ex.code == "not-found" || ex.problem == "not-found")


Can you explain this more? ex.code is from PackageKit, and ex.problem is from cockpit-bridge. The first is "not-found" when the package was not known to PackageKit, the latter is "not-found" when PackageKit itself isn't there.

Let's move this out into its own PR.

To be honest, I did not dig into details of this. Test was failing here and while debugging I watched for content of the ex. I will need to start over to give proper answer to your question.

Let's leave this out here, then.

mvollmer · 2018-06-14T12:57:02Z

pkg/apps/application-list.jsx

@@ -140,18 +140,20 @@ class ApplicationList extends React.Component {
            tbody = <tr className="app-list-empty"><td>{empty_caption}</td></tr>;
        } else {
            table_classes += " table-hover";
-            tbody = comps.map((c) => <ApplicationRow comp={c} />);
+            tbody = comps.map((c) => <ApplicationRow comp={c} key={c} />);


c is not a string. c.id should work.

mvollmer · 2018-06-14T12:58:02Z

pkg/apps/application.jsx

@@ -62,7 +62,7 @@ class Application extends React.Component {
        function render_homepage_link(urls) {
            return urls.map(url => {
                if (url.type == 'homepage') {
-                    return (<div className="app-links">
+                    return (<div className="app-links" key={url}>


url.link should work

mvollmer · 2018-06-14T12:58:56Z

pkg/apps/application.jsx

                } else if (paragraph.tag == 'ol') {
-                    return <ol>{paragraph.items.map(item => <li>{item}</li>)}</ol>;
+                    return <ol key={paragraph}>{paragraph.items.map(item => <li key={item}>{item}</li>)}</ol>;


Key is not a string.

fixed via indexes. Or anything better (natural key)?

mvollmer · 2018-06-14T12:59:33Z

pkg/apps/application.jsx

@@ -127,7 +127,7 @@ class Application extends React.Component {
                    {render_homepage_link(comp.urls)}
                    <div className="app-description">{render_description(comp.description)}</div>
                    <center>
-                        { comp.screenshots.map(s => <img className="app-screenshot" role="presentation" src={s.full} />) }
+                        { comp.screenshots.map(s => <img key={s} className="app-screenshot" role="presentation" src={s.full} />) }


Key is not a string.

mvollmer · 2018-06-14T12:59:38Z

pkg/apps/application.jsx

                } else {
-                    return <p>{paragraph}</p>;
+                    return <p key={paragraph}>{paragraph}</p>;


Key is not a string.

mareklibra · 2018-06-15T07:57:04Z

Fixed, can you have a look, please? I'm using array indexes to build the key string value. I believe, this might be ok in this case. If concerned, I can replace by either natural key (need to be found) or generally unique (using counter).

mvollmer · 2018-06-22T11:07:53Z

React related changes look good to me now, thanks!

The D-Bus fix should not be in this PR. It might or might not be a good fix, but it's unclear what it is fixing, and not related to React.

And fix <table><tbody> hierarchy for React.

mvollmer · 2018-07-06T12:37:57Z

I have removed the D-Bus fix from this PR.

mareklibra mentioned this pull request Jun 8, 2018

Switch to React from react-lite #9263

Merged

44 tasks

mareklibra changed the title ~~apps: Fix detection of DBus error~~ apps: React-related fixes Jun 8, 2018

martinpitt requested changes Jun 11, 2018

View reviewed changes

mareklibra mentioned this pull request Jun 11, 2018

check-storage-luks: Fixes for React #9355

Closed

martinpitt added the blocked Don't land until something else happens first (see task list) label Jun 11, 2018

martinpitt approved these changes Jun 11, 2018

View reviewed changes

mvollmer requested changes Jun 14, 2018

View reviewed changes

mareklibra force-pushed the react.apps branch from f2312fa to c4f141e Compare June 15, 2018 07:54

mvollmer removed the blocked Don't land until something else happens first (see task list) label Jul 6, 2018

apps: Add unique key to generated React component list

f1f9296

And fix <table><tbody> hierarchy for React.

mvollmer force-pushed the react.apps branch from c4f141e to f1f9296 Compare July 6, 2018 12:37

mvollmer approved these changes Jul 6, 2018

View reviewed changes

martinpitt merged commit 5046833 into cockpit-project:master Jul 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apps: React-related fixes #9344

apps: React-related fixes #9344

mareklibra commented Jun 8, 2018 •

edited by mvollmer

Loading

martinpitt Jun 11, 2018

martinpitt Jun 11, 2018

mareklibra Jun 11, 2018

mvollmer Jun 14, 2018

mareklibra Jun 15, 2018 •

edited

Loading

mareklibra commented Jun 11, 2018 •

edited

Loading

martinpitt left a comment

mareklibra commented Jun 11, 2018

mvollmer Jun 14, 2018

mareklibra Jun 15, 2018

mvollmer Jun 22, 2018

mvollmer Jun 14, 2018

mareklibra Jun 15, 2018

mvollmer Jun 14, 2018

mareklibra Jun 15, 2018

mvollmer Jun 14, 2018

mareklibra Jun 15, 2018

mvollmer Jun 14, 2018

mvollmer Jun 14, 2018

mareklibra commented Jun 15, 2018

mvollmer commented Jun 22, 2018

mvollmer commented Jul 6, 2018

apps: React-related fixes #9344

apps: React-related fixes #9344

Conversation

mareklibra commented Jun 8, 2018 • edited by mvollmer Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

mareklibra commented Jun 11, 2018 • edited Loading

martinpitt left a comment

Choose a reason for hiding this comment

mareklibra commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra commented Jun 15, 2018

mvollmer commented Jun 22, 2018

mvollmer commented Jul 6, 2018

mareklibra commented Jun 8, 2018 •

edited by mvollmer

Loading

mareklibra Jun 15, 2018 •

edited

Loading

mareklibra commented Jun 11, 2018 •

edited

Loading