Make WordPress Core

Opened 2 years ago

Closed 19 months ago

#55557 closed enhancement (fixed)

Reduce CSS-float in admin

Reported by: kebbet's profile kebbet Owned by:
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch
Focuses: css Cc:

Description (last modified by SergeyBiryukov)

Improve the CSS-code with less floating, as float never was intended for layout.

Extension and a follow up to #55498 where available structure tags are floated in a horizontal line.

Attachments (8)

55557-permalink-tags.diff (2.1 KB) - added by kebbet 2 years ago.
Patch for permalink structure tags.
55557-timezone-info.diff (608 bytes) - added by kebbet 2 years ago.
Flex in timezone-info.
55557-search-box-inline-edit.diff (4.1 KB) - added by kebbet 2 years ago.
Search input in fieldset. Flex inline edit. (as fieldset-rule for search field interfere with inline edit)
55557-permalink-tags.2.diff (663 bytes) - added by kebbet 21 months ago.
Only change CSS for permalinks screen.
Capture d’écran 2022-09-16 à 00.35.19.png (293.5 KB) - added by audrasjb 19 months ago.
Testing 55557-timezone-info.diff — before patch
Capture d’écran 2022-09-16 à 00.35.43.png (301.8 KB) - added by audrasjb 19 months ago.
Testing 55557-timezone-info.diff — after patch — Works fine!
timezone-local-before.png (8.9 KB) - added by sabernhardt 19 months ago.
Timezone settings with local time, before patch
timezone-local-with-patch.png (9.7 KB) - added by sabernhardt 19 months ago.
Timezone settings with local time, applying 55557-timezone-info.diff

Download all attachments as: .zip

Change History (32)

This ticket was mentioned in PR #2558 on WordPress/wordpress-develop by kebbet.


2 years ago
#1

  • Keywords has-patch added; needs-patch removed

#2 @costdev
2 years ago

  • Milestone changed from Awaiting Review to 6.1
  • Type changed from enhancement to task (blessed)

I agree that we should try to reduce the use of float on an ongoing basis. This also helps to modernise the CSS used in Core.

I'm making this a Task and moving it to the 6.1 milestone, as changes will need testing and we're already in Beta for 6.0.

I think it's best to submit .diff/.patch files for each change/group of related changes to make it easier to review, test, commit and, if necessary, revert.

@kebbet
2 years ago

Patch for permalink structure tags.

@kebbet
2 years ago

Flex in timezone-info.

@kebbet
2 years ago

Search input in fieldset. Flex inline edit. (as fieldset-rule for search field interfere with inline edit)

#3 @kebbet
2 years ago

All patches tested on single and multisite installs.

kebbet commented on PR #2558:


2 years ago
#4

Branch is used to genereate .diff-files.

#5 @SergeyBiryukov
23 months ago

  • Description modified (diff)

#6 @audrasjb
21 months ago

@kebbet first, 55557-permalink-tags.diff:

The change looks good to me when applied on trunk.
I'm only wondering about why you switched list items with a semantic markup to a generic div.

55557-timezone-info.diff looks good to go.
Same goes for 55557-search-box-inline-edit.diff.

#7 @kebbet
21 months ago

@audrasjb thanks for reviewing!

Technically it’s not a list, and a wrapping element is not needed for the buttons. The buttons can be flexed inside a container, and a div works for that case. I used a <div> so the <li> elements can be removed.

#8 @audrasjb
21 months ago

You're welcome @kebbet!

Although I'm not sure it's not a list semantically, then we should also remove the role="list" attribute :)

#9 @kebbet
21 months ago

Haha, then let’s remove that too.

#10 @sabernhardt
21 months ago

Hundreds of plugins use the selector p.search-box, and the fieldset element does not add much semantic value when it only contains one input field. Then again, at least one of those plugins assigns a float on the p.search-box selector.

Also, the role="list" was added to the ul tag to make sure the permalink structure tags button set would be read as a list (ticket:29872#comment:25). We could reconsider that, but I like having the items counted.

Last edited 21 months ago by sabernhardt (previous) (diff)

#11 @kebbet
21 months ago

Very good points @sabernhardt! Then maybe not change the searchfield now.

A novice question: Is there any contradiction in using role="list" on a <div>-element?

#12 @audrasjb
21 months ago

The list role is often used to add semantics to elements that doesn't convey any semantic, so that's especially helpful in this case :)

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/list_role

#13 @sabernhardt
21 months ago

When a div element has the list role, then its children should have the `listitem` role (or else group).

If we retain the roles that ul and li have natively, I would rather not replace the semantic elements.

The question is whether the list roles are necessary or beneficial in this case. I think these roles are unnecessary but nice to have.

CSS flex seems to work with the list tags, though it probably would require overriding the li margins.

#14 @kebbet
21 months ago

  • Keywords needs-refresh added

So there are two ways forward I guess:

  • keep current HTML-markup and only change css to use flex instead of float for positioning.
  • or update the permalink-patch to use role="listitem" on the button-elements.

I vote for add role="listitem" since it's a nice to have, and we don't have to deal with overriding the default margins on li-elements.

#15 @audrasjb
21 months ago

Both options work for me :)

#16 @sabernhardt
21 months ago

No, button elements need to keep their button role. That's more important than the list.

Without changing the markup, I can get the same margins that the PR has by replacing the old li float and margin with this:

.form-table.permalink-structure .available-structure-tags ul {
	display: flex;
	flex-wrap: wrap;
	margin: 6px 0 0;
}
.form-table.permalink-structure .available-structure-tags li {
	margin: 8px 8px 0 0;
}
.form-table.permalink-structure .available-structure-tags li:last-child {
	margin-right: 0;
}

#17 @kebbet
21 months ago

Well then let's go for no markup change this time, and only adjust the CSS for current markup. I was not aware of role the ul/li-elements actually played apart from being the containers for the buttons.
Thanks for the insights @sabernhardt, really appreciated!

@kebbet
21 months ago

Only change CSS for permalinks screen.

#18 @kebbet
21 months ago

  • Keywords needs-refresh removed

Okay, so two patches needs reviews now and are possible candidates for including in core.

  • 55557-timezone-info.diff
  • 55557-permalink-tags.2.diff

@audrasjb
19 months ago

Testing 55557-timezone-info.diff — before patch

@audrasjb
19 months ago

Testing 55557-timezone-info.diff — after patch — Works fine!

#19 @audrasjb
19 months ago

I tested 55557-timezone-info.diff and this one looks good to go :)

#20 @audrasjb
19 months ago

In 54185:

Code Modernization: Reduce CSS float usage in wp-admin - Timezone settings.

This changeset is a part of a task dedicated to improve wp-admin CSS code with less floating, as float never was intended for layout. The idea is to gradually replace floating methods that take the HTML element out of the normal flow of the document with more modern and robust positioning methods.

Props kebbet.
See #55557.

@sabernhardt
19 months ago

Timezone settings with local time, before patch

@sabernhardt
19 months ago

Timezone settings with local time, applying 55557-timezone-info.diff

#21 @sabernhardt
19 months ago

The screenshots needed a local time to show how the two times are now in columns instead of inline (they weren't floating).

That said, r54185 works fine.

#22 @audrasjb
19 months ago

Ah yes thanks for adding more context. That said, I think we can say that we're fine with this change, indeed :)

#23 @davidbaumwald
19 months ago

  • Type changed from task (blessed) to enhancement

This would be an Enhancement not an officially Task (Blessed).

#24 @sabernhardt
19 months ago

  • Resolution set to fixed
  • Status changed from new to closed

I moved the permalinks part to #56673, so we can close this as fixed with r54185.

Note: See TracTickets for help on using tickets.