-
Notifications
You must be signed in to change notification settings - Fork 74
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
First batch of React error fixes #1270
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
56e0e45
to
b01a5e4
Compare
a027d10
to
7f5e607
Compare
Replace assertTrue() with better alternatives to make failures useful for debugging. Fix the completely broken assertion in TestMachinesSettings, which was previously not actually doing the intended "cdrom" check.
All three calls in initState() are async. The App initialization must wait at least until getLoggedInUser() finished, as CreateStoragePoolAction requires the `loggedUser` property, which comes from the global state. Presumably this is true for a lot of other properties as well. So keep the "loading resources" screen until initState() finished, and also start with "loadingResources" right away to avoid an initial rendering of components.
All other bugs got fixed, but that one is hard. See issue cockpit-project#1272 for details.
This is non-trivial to fix, see issue cockpit-project#1273 for the details.
`cacheMode` is null if the domain XML does not specify a value, but the `value` should always point to an actual valid option. Fall back to "default".
This is a string, not a boolean property, so avoid a value of `false` if `keyInvalid` is falsy.
This picks up dropping the unknown `noVerticalAlign` from HelpIcon.
This fixes a React error: > Warning: A future version of React will block javascript: URLs as a security precaution. > Use event handlers instead if you can. And yes, we can! Add a stub handler to keep the label focusable.
This changed in PF5.
7f5e607
to
e7bf5c8
Compare
/devel succeeded, so re-running with full tests. This is a good first batch for landing. |
@@ -159,7 +159,10 @@ export const VmNeedsShutdown = ({ vm }) => { | |||
position="bottom" | |||
hasAutoWidth | |||
bodyContent={body}> | |||
<Label className="resource-state-text" href="javascript:null" color="blue" id={`vm-${vm.name}-needs-shutdown`} icon={<PendingIcon />}>{_("Changes pending")}</Label> | |||
<Label className="resource-state-text" color="blue" id={`vm-${vm.name}-needs-shutdown`} | |||
icon={<PendingIcon />} onClick={() => null}> |
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 added line is not executed by any test. Details
@@ -159,7 +159,10 @@ export const VmNeedsShutdown = ({ vm }) => { | |||
position="bottom" | |||
hasAutoWidth | |||
bodyContent={body}> | |||
<Label className="resource-state-text" href="javascript:null" color="blue" id={`vm-${vm.name}-needs-shutdown`} icon={<PendingIcon />}>{_("Changes pending")}</Label> | |||
<Label className="resource-state-text" color="blue" id={`vm-${vm.name}-needs-shutdown`} | |||
icon={<PendingIcon />} onClick={() => null}> |
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.
Wondering if dropping onClick
is enough?
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 added it. It needs to have either a href=
or an onClick
handler to become focusable, and I think that was the original intent for that hack.
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 the cleanups!
This starts to address or at least confine the React errors that were spotted by fixing the console.error() checks in PR #1269