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.

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

Fix DOM nesting on the Storage page dialogs, enable DOM nesting errors #19509

Merged
merged 5 commits into from
Oct 26, 2023

Conversation

martinpitt
Copy link
Member

@martinpitt martinpitt commented Oct 19, 2023

We have the following cases in main:

storage:

  • TestStorageLuks.testLuks: race condition, hard to investigate; not in dialog, wait for storage rewrite, ignore
  • TestStorageLvm2.test{Lvm,Snapshots}: Grow button is in "dd > div > dd" without a new list wrapper; affected by page reorg, postpone and ignore for now
  • TestStorageMounting.testMounting, TestStorageNfs.testNfsBusy: in "used by pid/service" dialog, let's fix
  • TestStorageLuksDestructive.testLuks1Slots: in dialog, let's fix
  • TestStorageNBDE.test{Basic,RootReboot}: In dialog, let's fix
  • TestStorage.testUsed - in dialog, let's fix

expensive/networking already got fixed in #19520:

/other is okay.

Builds on top of:

@martinpitt martinpitt added no-test For doc/workflow changes, or experiments which don't need a full CI run, blocked Don't land until something else happens first (see task list) labels Oct 19, 2023
@martinpitt martinpitt changed the title test: WIP check DOM nesting Fix DOM nesting Oct 20, 2023
@martinpitt martinpitt removed the blocked Don't land until something else happens first (see task list) label Oct 20, 2023
@martinpitt
Copy link
Member Author

After round 1: The ListingTable nesting fix caused some test regressions, selectors need to be adjusted. Some of the storage failures are also due to that, and some are independent problems. /expensive and /networking are green now.

@martinpitt martinpitt added the blocked Don't land until something else happens first (see task list) label Oct 24, 2023
@martinpitt martinpitt changed the title Fix DOM nesting Fix DOM nesting on the Storage page dialogs, enable DOM nesting errors Oct 24, 2023
@martinpitt martinpitt temporarily deployed to gh-cockpituous October 24, 2023 10:24 — with GitHub Actions Inactive
@martinpitt martinpitt temporarily deployed to gh-cockpituous October 25, 2023 04:27 — with GitHub Actions Inactive
@martinpitt martinpitt removed the blocked Don't land until something else happens first (see task list) label Oct 25, 2023
A `<table>` must not appear inside of a `<p>`, make them siblings
instead.
A `<List>` block element must not appear inside a `<p>`, make them
siblings instead.
RemovePassphraseField is already in a `<Form>`, so don't wrap the
checkbox into a second one.
A ClipboardCopy is a block element (`<div>`) and thus cannot be put into
a `<p>`. Make them silblings. To retain the current layout, turn the
text into a simple span, so that the text and the ClipboardCopy still
appear side by side.
Most errors were fixed recently. The remaining ones on the Storage page
conflict with the ongoing page redesign, and may well already be fixed
by it. Ignore them specifically for now.

This will prevent introducing new DOM nesting errors in the future.
@martinpitt martinpitt removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 25, 2023
@martinpitt martinpitt temporarily deployed to gh-cockpituous October 25, 2023 06:05 — with GitHub Actions Inactive
@martinpitt martinpitt marked this pull request as ready for review October 25, 2023 07:16
<Checkbox id="force-remove-passphrase"
isChecked={val !== false}
label={_("Confirm removal with an alternate passphrase")}
onChange={(_event, checked) => change(checked ? "" : false)}
Copy link
Contributor

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

@martinpitt
Copy link
Member Author

Rawhide failure is unrelated, see PR #19525

@mvollmer mvollmer merged commit 5be680e into cockpit-project:main Oct 26, 2023
99 of 100 checks passed
@martinpitt martinpitt deleted the dom-nesting branch October 26, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants