DataViews grid layout: Fix long pattern names display #65200
+26
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akasunil
added
[Type] Bug
|
Size Change: +146 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
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. |
jameskoster
requested changes
Sep 10, 2024
t-hamano
reviewed
Sep 11, 2024
There was a problem hiding this comment.
In addition to @jameskoster's suggestion, I think the two-line text should only apply to the grid layout.
The table layout has enough width, so I think it's better to keep it one line.
Is this design works? Basically, I oriented all objects to the top and increased the line height of the text to 24px, which matches the size of the icons on the right side. This will make the text center aligned with the icon. |
Did you push this change, I'm not seeing it? The line-height might be a bit too relaxed but it's hard to say without testing. I'm also unsure about the inconsistent gaps between title + field. I made an alternative mockup in Figma: 
What do you think? |
|
I also noticed that patterns on the 'My patterns' section are still being truncated. |
No, i didn't, I was playing directly in browser's inspector.
How did you managed to to center it vertically without altering line height to match with icons height which is 24px? Are we adding top margin? Also, i requested for edit access in above figma to inspect the styles. |
|
Line-height is 20px. There's padding on the element containing the title + actions button: 
Then some tiny padding on the title itself to increase it's effective min-height to 24px so that it's aligned with the icon when there's only a single line: 
|
…rn-title-text-overflow-65134
…rn-title-text-overflow-65134
|
Hi @jameskoster, I have update the design. could use your review. 
|
|
Flaky tests detected in d91af64. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11914714395
|
|
Thank you, this looks much better. If I may suggest one final tweak; let's increase the bottom padding on I noticed that this treatment is not applied on the "My patterns" view. Is that intentional?
|
t-hamano
reviewed
Sep 25, 2024
| size="compact" | ||
| size="small" |
There was a problem hiding this comment.
@jameskoster Is the icon size change (from 32px to 24px) something to expect in this PR? It should affect all layouts.
There was a problem hiding this comment.
Good spot, no this should only affect the grid layout.
There was a problem hiding this comment.
Should i change it with styles to only impact on grid view?
Comment on lines
107
to
121
| .dataviews-view-grid { | ||
| .edit-site-patterns__pattern-title { | ||
| display: -webkit-box; | ||
| -webkit-line-clamp: 2; | ||
| -webkit-box-orient: vertical; | ||
| white-space: normal; | ||
| padding-top: 3px; | ||
| align-self: start; | ||
| } | ||
|
|
||
| .edit-site-patterns__pattern-lock-icon { | ||
| align-self: start; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This may be a personal preference, but you may find the following writing style easier to read:
.edit-site-patterns__pattern-title {
.dataviews-view-grid & {
}
}
.edit-site-patterns__pattern-lock-icon {
.dataviews-view-grid & {
}
}It's similar to the writing style here:
gutenberg/packages/edit-site/src/components/page-templates/style.scss
Lines 26 to 38 in b202059
|
Noticed the titles on the Templates page are centrally aligned on this PR which looks a little clunky: 
|
…rn-title-text-overflow-65134
…rn-title-text-overflow-65134
My pattern have button instead of plain text on titles.I added extra CSS to it to make it consistent with the overall pattern design. |
…rn-title-text-overflow-65134
|
I have filed #66733 to resolve some of the issues, to make it easier to move this PR forward. |
t-hamano
reviewed
Nov 5, 2024
Comment on lines
127
to
131
| .components-button.dataviews-all-actions-button.has-icon:not(.has-text) { | ||
| .dataviews-view-grid & { | ||
| min-width: 24px; | ||
| height: 24px; | ||
| } |
There was a problem hiding this comment.
It is better to avoid using selectors with component prefixes whenever possible. Additionally, the focus outline is not square:
Perhaps it would be better to add a prop to the CompactItemActions component to change the size only when in grid layout. I'll consider addressing this in a separate PR.
t-hamano
reviewed
Nov 14, 2024
There was a problem hiding this comment.
Thanks for the update!
The current layout of the primary field is incorrect, so I've submitted #66995 to fix that. Additionally, the action button should be determined via size prop instead of having its size hardcoded.
I'll come back to this PR after I've addressed these two issues.
t-hamano
reviewed
Nov 19, 2024
There was a problem hiding this comment.
The current layout of the primary field is incorrect, so I've submitted #66995 to fix that. Additionally, the action button should be determined via
sizeprop instead of having its size hardcoded.I'll come back to this PR after I've addressed these two issues.
I have resolved these issues:
- DataViews: Tweak primary field in patterns grid layout #66733
- DataViews: Reduce the size of action button in Grid layout #67032
@akasunil Could you rebase this PR? Sorry for the wait 🙇♂️
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #65134
Why?
Long pattern names are cut off when the grid layout is used
Testing Instructions
Screenshots or screencast