DataViews: Support combined fields #63236
Conversation
youknowriad
added
[Type] Enhancement
|
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. |
|
Size Change: +787 B (+0.04%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
If we're going to force the description to be visible in table layout – as this PR does – then the 'Description' toggle in view options feels superfluous. I think the toggle should either be removed (only for table layout), or connected with the field inside the "Template" column. The latter would obviously be more flexible, but probably trickier to implement? One minor detail; if the description doesn't live in it's own column, then we no longer need to show the |
|
@jameskoster That's a good point indeed, and I think it might take a few PRs to get everything right here. For start, I'm now hiding all fields that are part of the "combined fields". |
|
Functionality-wise this works for me, for now :) In terms of styling I have a small request;
I wonder if we should restrict the width of the description somehow, legibility on widescreens is a bit awkward: 
Do you think it would be best to do that on the whole column, or just the description field? |
|
On "widths" configurations. We do have "widths" defined on fields but for now I'm ignoring them in the PR because I think this was not the right implementation. These widths don't make sense in other layouts, so they should mostly be layout specific configurations. The work I'm doing here #63287 will allow us to define these "layout + field" specific rendering configs more easily. |
youknowriad
force-pushed
the
add/combined-fields
branch
from
82abb88
to
85d5da2
Compare
|
@jameskoster now that the other PR landed, I rebased this one and I restored the ability to set "widths" for columns. Let me know if it works as you expect. In my testing, setting min/max widths didn't really any meaningful impact but I'm not sure if that's very different than trunk. |
|
@youknowriad The preview field seems to have moved to the end of the row: 
The column width doesn't seem to have any affect, I suppose that's just the nature of tables. Maybe the description field should include a max-width. Probably fine to look into that separately. One other observation is that the space between title + description is different based on the number of lines: 
It's caused by the Something like this would fix: Though we probably need something more specific than |
I was wondering when you'll discover that :P. It's not just the end, depending on the order of when you trigger the visibility of the fields, they may appear in different orders. The reality here is that there's no way around that aside providing a way to reorder fields in the UI. We can either decide to ship this PR as is or pause and implement field reordering first in trunk and come back to this PR afterwards.
I found that setting "width" actually is better than max/min widths but yeah it's the same as trunk, so we should explore it separately.
Can you explain the purpose of these styles so I can try to think about the right fix. |
Sneaky 🥷 So there's no way to define a default field order without also building re-ordering? If that's the case then we may need to prioritise re-ordering. It seems a bit unexpected for the Preview to appear at the end of the row. Interested to hear @jasmussen's thoughts on that though.
Additionally, when the row contains a field that is taller than the min-height, everything keeps alignment at the top of the row: 
|
I can try some temporary solutions but nothing that I like. I do think we need to prioritize reordering. |
The main issue is that we can use the "fields" definition as the default order, but it doesn't work when you inject "virtual fields" like "template" in this PR. So we need to either consider the "template" as last or first item when toggling visibility. |
This is not something that can be codified and I'm not entirely sure how to solve this issue to be honest. |
|
And for the reordering issue, I'd rather solve it first as trying to fit something here would be too hacky. So what do you think we should do? |
|
I pushed a small fix for the line height, I think it's not a great fix as these rules are very fragile and the don't scale/compose well when the tables get complex but I can live with it for now. |
|
If re-ordering isn't a huge lift then I suppose it makes sense to prioritise. The design should be relatively simple: 
I agree, but I'm hesitant to admit defeat and resort to 
Tables... 😓 |
|
Ok, so what do you think about shipping this PR as is and I can work on reordering as a follow-up. The designs look easy enough to implement. |
|
Sounds like a plan 👍 I suspect we may also need to prioritise #57669. It would be annoying to have to toggle visibility and order frequently. Or will the re-ordering function make it possible to set a default order too? |
Default order when you first load the layout but not when you enable fields. I think that would be too much. |
| if ( view.type === 'table' ) { | ||
| return [ view.layout?.primaryField ] | ||
| .concat( | ||
| view.layout?.combinedFields?.flatMap( |
There was a problem hiding this comment.
Why should we consider combined fields as mandatory? 🤔
There was a problem hiding this comment.
Because we don't have the right UI and components to actually disable them. If you manage to make them work without issues, you can remove this condition :)
Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Related to #55083
Follow-up to #63127
What?
This PR implements combining fields in table views. It allows combining one or more fields horizontally or vertically in table views. To do so, you have to define a "combinedField" in
view.layout.combinedFieldsFor instance:and then you can update the
fieldsproperty to include thetemplatefield.This doesn't work entirely well in the fields visibility menu.
I was considering an alternative approach that was way bigger (See #63148) but now I'm thinking the refactoring is not worth doing yet as there's still a lot of unknowns. So this PR implements the combination with the minimal changes.
Testing instructions