packagekit: Swap out many divs and spans for React.Fragment #9309
Conversation
|
This isn't working properly at the moment. At least I did get it compiling, however. 🤷♂️ Is it due to react-lite? or something else? |
pkg/packagekit/updates.jsx
Outdated
| @@ -172,9 +172,9 @@ function HeaderBar(props) { | |||
| actionButton = <button className="btn btn-default" onClick={props.onRefresh} >{_("Check for Updates")}</button>; | |||
| if (props.timeSinceRefresh !== null) { | |||
| lastChecked = ( | |||
| <span style={ {paddingRight: "3ex"} }> | |||
There was a problem hiding this comment.
There shouldn't be inline CSS. This should be a CSS rule in the stylesheet. (I have it fixed in my mobile changes.)
pkg/packagekit/updates.jsx
Outdated
| )); | ||
|
|
||
| return <div>{paragraphs}</div>; | ||
| return <React.Fragment>{paragraphs}</React.Fragment>; |
There was a problem hiding this comment.
Should this just be the following?
return paragraphs;
There was a problem hiding this comment.
Yes, I think returning a list of elements is actually legitimate and works. I suppose I just didn't know about that yet when I wrote this. In the same vein, if Fragment does not work with react-lite, the other cases could actually use the same trick? I. e. return a (static) array?
There was a problem hiding this comment.
returning an array, no matter if static or generated, works fine. Just the key prop needs to be set
|
The |
|
@garrett , to avoid conflicts and make this directly with the Btw, returning an array of elements work, as already used on other places as a workaround, but it breaks React component hierarchy since you need to introduce a helper function providing such an array. The function can't be a React component. |
|
@mareklibra: After #9263 is merged, we can go thought and fix things properly (outside of this PR). I'd suggest we stick to the shorthand, |
That's the point, react-lite implements v15 (and not entirely). There's nothing newer between recent react-lite and the #9263 . Anyway, I'm fine with recent workaround via array of elements. Just seems like a double-work from my POV. |
|
@mareklibra The problem is that if we don't go with a new and official enough version of React, then there's tag soup, with divs and spans all over the place — none of which have any semantic meaning, and some can definitely get in the way of various types of layout too. Without actual meaning, this makes accessibility much harder to properly implement. All the div and span nesting increases the DOM node usage, especially when in loops, making the page more complex and slower overall. This is especially noticeable in loadtime, rendertime, memory usage, with CSS, and when using JavaScript (especially JavaScript DOM traversing and manipulation). |
|
(So, yeah, even just for fragments, I'm already sold on switching away from React lite.) |
|
This is now unblocked, React switch landed (PR #9263) |
pkg/packagekit/updates.jsx
Outdated
| { cockpit.format(_("Last checked: $0 ago"), moment.duration(props.timeSinceRefresh * 1000).humanize()) } | ||
| </span> | ||
| </React.Fragment> |
There was a problem hiding this comment.
This is just a single text element, so I don't think it needs to be a fragment at all. The span was just done for the paddingRight, but that's now gone anyway (and probably never worked because of unsafe-inline CSP)
garrett
force-pushed
the
react-fragment
branch
4 times, most recently
from
6dd6165 to
a704cc1
Compare
martinpitt
force-pushed
the
react-fragment
branch
from
a704cc1 to
a36bcf2
Compare
martinpitt
changed the title
|
@martinpitt: Nice! Thanks! |
No description provided.