Opened 4 years ago
Closed 3 years ago
#55557 closed enhancement (fixed)
Reduce CSS-float in admin
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | 6.1 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Administration | Keywords: | has-patch |
| Focuses: | css | Cc: |
Description (last modified by )
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)
Change History (32)
This ticket was mentioned in PR #2558 on WordPress/wordpress-develop by kebbet.
4 years ago
#1
- Keywords has-patch added; needs-patch removed
#2
@
4 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.
@
4 years ago
Search input in fieldset. Flex inline edit. (as fieldset-rule for search field interfere with inline edit)
#6
@
4 years 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
@
4 years 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
@
4 years 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 :)
#10
@
4 years 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 it was read as a list (ticket:29872#comment:25). We could reconsider that, but I like having the items counted.
#11
@
4 years 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
@
4 years 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
@
4 years 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
@
4 years 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 thebutton-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.
#16
@
4 years 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
@
4 years 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!
#18
@
4 years 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
#21
@
3 years 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
@
3 years ago
Ah yes thanks for adding more context. That said, I think we can say that we're fine with this change, indeed :)
Very much WIP
https://core.trac.wordpress.org/ticket/55557