Remove background-color and box-shadow from actions #65218
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
colorful-tones
added
the
[Feature] DataViews
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
@jasmussen do you have insight on how to treat this small bit of UI in DataViews please? |
I remembered it’s for long titles:
So that’s probably going to make this trickier. |
|
Yep, it exists for the careful truncation, and I believe @jameskoster worked on these pieces. Agree we need a systemic approach to removing hard-coded colors. I believe this is one of the reasons the theme package (#56669) is so important to land. CC: @ciampo |
|
Without these styles actions and titles will overlap: 
Hard-coded values are not great, but they're worth suffering to avoid the outcome above. Maybe there's a different way to approach this that maintains truncation and keyboard navigation? |
There was a problem hiding this comment.
Yep, unfortunately, this won't work as-is.
But what if we add some padding to .edit-site-post-list__title span? Adding padding-right: 70px to it for example does the job because we've already got overflow: hidden and text-overflow: ellipsis:
If the padding works, removing the background and shadow should work just fine as there will no longer be any overlap.
|
As @tyxla said, text truncation seems to be already in place, but the overall size of the containing element needs to be adjusted to reflect the screen estate that we want the text to occupy. |
|
I wanted to suggest the cheapest solution would be a CSS variable for the background/box-shadow to use. It works for the hover state but the selected state’s tint is mostly transparent so doesn’t work to "truncate" the title. It could be made to work if that tint of the admin theme color was a luminosity tweak instead of alpha but I'm not sure that could be easily done at the moment. So shortening the title in hover and select states seems more straightforward (and nicer too in that it keeps the ellipsis visible). Diff to truncate moar on hover/selectdiff --git a/packages/dataviews/src/dataviews-layouts/list/index.tsx b/packages/dataviews/src/dataviews-layouts/list/index.tsx
index 8a3f6a2973..bddf7de090 100644
--- a/packages/dataviews/src/dataviews-layouts/list/index.tsx
+++ b/packages/dataviews/src/dataviews-layouts/list/index.tsx
@@ -6,7 +6,7 @@ import clsx from 'clsx';
/**
* WordPress dependencies
*/
-import { useInstanceId, usePrevious } from '@wordpress/compose';
+import { useInstanceId, usePrevious, useRefEffect } from '@wordpress/compose';
import {
__experimentalHStack as HStack,
__experimentalVStack as VStack,
@@ -144,12 +144,14 @@ function ListItem< Item >( {
const labelId = `${ idPrefix }-label`;
const descriptionId = `${ idPrefix }-description`;
+ const [ actionsWidth, setActionsWidth ] = useState( 0 );
+ const measureActionsWidth = useRefEffect< HTMLElement >( ( node ) => {
+ setActionsWidth( node.offsetWidth );
+ }, [] );
const [ isHovered, setIsHovered ] = useState( false );
- const handleMouseEnter = () => {
- setIsHovered( true );
- };
- const handleMouseLeave = () => {
- setIsHovered( false );
+ const handleHover: React.MouseEventHandler = ( { type } ) => {
+ const isHover = type === 'mouseenter';
+ setIsHovered( isHover );
};
useEffect( () => {
@@ -186,6 +188,10 @@ function ListItem< Item >( {
const renderedPrimaryField = primaryField?.render ? (
<primaryField.render item={ item } />
) : null;
+ const primaryFieldInlineStyle =
+ isHovered || isSelected
+ ? { paddingInlineEnd: actionsWidth }
+ : undefined;
return (
<Composite.Row
@@ -196,8 +202,8 @@ function ListItem< Item >( {
'is-selected': isSelected,
'is-hovered': isHovered,
} ) }
- onMouseEnter={ handleMouseEnter }
- onMouseLeave={ handleMouseLeave }
+ onMouseEnter={ handleHover }
+ onMouseLeave={ handleHover }
>
<HStack
className="dataviews-view-list__item-wrapper"
@@ -230,6 +236,7 @@ function ListItem< Item >( {
<span
className="dataviews-view-list__primary-field"
id={ labelId }
+ style={ primaryFieldInlineStyle }
>
{ renderedPrimaryField }
</span>
@@ -267,6 +274,7 @@ function ListItem< Item >( {
flexShrink: '0',
width: 'auto',
} }
+ ref={ measureActionsWidth }
>
{ primaryAction && (
<PrimaryActionGridCell
diff --git a/packages/dataviews/src/dataviews-layouts/list/style.scss b/packages/dataviews/src/dataviews-layouts/list/style.scss
index 47847967af..8fd6991173 100644
--- a/packages/dataviews/src/dataviews-layouts/list/style.scss
+++ b/packages/dataviews/src/dataviews-layouts/list/style.scss
@@ -23,6 +23,8 @@ ul.dataviews-view-list {
position: absolute;
top: $grid-unit-20;
right: 0;
+ // Pads __primary-field’s text truncation on hover (done in JS; see ./index.tsx).
+ padding-inline-start: $grid-unit-10;
> div {
@@ -45,7 +47,6 @@ ul.dataviews-view-list {
&.is-hovered,
&:focus-within {
.dataviews-view-list__item-actions {
- padding-left: $grid-unit-10;
margin-right: $grid-unit-30;
.components-button {
What it looks like: list-item-hover-truncate.mp4Obviously this could be simpler by just truncating that amount in all states but if that was acceptable I don’t know why it wouldn’t have been done that way already. |
I overlooked the keyboard navigation so that diff from my last comment lacks that but shouldn’t be hard to add in. |
|
I updated and applied @stokesman suggestion.
I'm not sure what aspect of keyboard navigation is missing? It seems like the tab/focus order is fine. 🤔 |
|
Not a blocking piece of feedback, but the proposed solution feels a bit overengineered to me, and it also duplicates the layout logic (which is now defined both in JS and in CSS). Also, the current JS-based solution wouldn't work as expected if, for any reason, the actions container width changes at runtime. |
Just that focus (without a click) currently won’t cause the padding to be applied so the title and actions can overlap. I suppose any effort on a fix may best be spared until it’s determined if a better approach is possible. I agree with the general sentiment Marco expressed. It seems to me, the go-to solution would be to revise the layout so the title and actions widths are dependent. I think I assumed there’s some catch to that since it’s not that way already. Perhaps someone has some knowledge about that. |
Yup, this is a regression:
Possibly unrelated, but another important detail here was to avoid nesting interactive elements (in this case |
|
Ok, it seems that maybe my efforts are inadequate at this point. Shall I close out this PR and let someone else take a pass at a suitable solution? |
Yes, that certainly requires some layout trickery to achieve the design. I did try tinkering on an alternate layout that seems to have promise. It is a bigger effort than the this PR’s current approach but it doesn’t need JS hackery. I found #63299 to be a nice reference for this effort in general. There’s an interesting point regarding the selected background color brought up there:
Originally posted by @jameskoster in #63299 (comment) If that were available, it should unblock a fairly minimal resolution for this.
If you’d like to. I find this acceptable as short-term fix—assuming the keyboard focus can be corrected simply enough. Though I suppose the issue isn’t so grave that it requires fixing before the next release. I suspect more suitable fixes will likely miss the upcoming release. |
|
I tried exploring some other color options, but the closest I could get was utilizing older Sass variables, e.g.
I agree with @jasmussen , and at this point, we'll need the new admin theme config to land to propagate and remove the older color systems. @stokesman feel free to explore layout solutions, but I'll close this PR out. |
Pull request was closed
For the sake of folks maybe still subscribed here, I’ve tried the refactored layout solution in #65376. |
What?
Addresses #65215. The hard-coded colors are not ideal. I've removed the
background-colorandbox-shadowaltogether, because it seemed like they were unnecessary for.is-selectedand even:hover,focus-within. If there is a Figma design for this or further insight on what/why this small UI needs bg then let me know and I'm happy to adjust.Why?
See #65215
How?
Remove CSS for bg color and box shadow for the small actions area.
Testing Instructions