Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataViews: fix field reordering and visibility logic #64999

Merged
merged 12 commits into from
Sep 3, 2024

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Sep 3, 2024

Part of #55083

What & Why?

This PR fixes a few issues around field reordering & visibility:

  • Displays moving controls for combined fields. Combined fields could be reordered via the view config popup but lacked the same controls in the table header.
  • Prevent fields that are part of a combined field from being displayed when they are listed in view.fields.
  • Do not display hidden fields that cannot be made visible in view config.
  • When view.fields is not provided: reordering fields should work.
  • When view.fields is not provided: the properties panel in view config should be visible. Reordering & visibility was still available via the table header.
  • When view.fields is not provided: display combined fields properly.

How?

Refactored the logic that deals with visibility & reordering fields.

Testing Instructions

Displays moving controls for combined fields

  • Go to "Site editor > Templates" and switch to table layout.
  • Verify the reordering controls are visible for combined fields.
Before After
240903-table-header-controls-for-combined-fields-before 240903-table-header-controls-for-combined-fields-after

Prevent fields that are part of a combined field from being displayed when they are listed in view.fields

  • Add title in view.fields for the Templates Page.
  • Go to "Site editor > Templates" and switch to table layout.
  • The expected result is that the title field (with Template label) is not visible in the table and the view config properties panel.
Before After
240903-combined-fields-in-view-fields-before 240903-combined-fields-in-view-after

Do not display hidden fields that cannot be made visible in view config

  • npm install && npm run storybook:dev.
  • Go to the default story for DataViews in storybook and verify the type field is not listed in hidden fields.
Before After
240903-hidden-fields-not-toggeable-before 240903-hidden-fields-not-toggeable-after

When view.fields is not provided: reordering fields should work.

  • Remove view.fields and view.layout.combinedFields for the Templates Page.
  • Go to "Site editor > Templates" and switch to table layout.
  • The expected result is that reordering fields works.

Before

240903-table-header-with-no-view-fields-before.mov

After

240903-table-header-with-no-view-fields-after.mov

When view.fields is not provided: the properties panel in view config should be visible

  • Remove view.fields and view.layout.combinedFields for the Templates Page.
  • Go to "Site editor > Templates" and switch to table layout.
  • The expected result is that the properties panel within the view config is visible.
Before After
240903-view-config-with-no-view-fields-before 240903-view-config-with-no-view-fields-after

When view.fields is not provided: display combined fields properly

  • Remove view.fields for the Templates Page.
  • Go to "Site editor > Templates" and switch to table layout.
  • The expected result is that both the table & the properties panel display the proper fields (the combined template plus preview and author which are not part of any combination):
Before After
Captura de ecrã 2024-09-03, às 12 47 43 Captura de ecrã 2024-09-03, às 12 47 28

Copy link

github-actions bot commented Sep 3, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: oandregal <oandregal@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@oandregal oandregal added [Type] Bug An existing feature does not function as intended [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Sep 3, 2024
@oandregal oandregal self-assigned this Sep 3, 2024
Copy link

github-actions bot commented Sep 3, 2024

Flaky tests detected in 2c2d801.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10681307930
📝 Reported issues:

@oandregal
Copy link
Member Author

This actually stems from wanting to fix this issue #64381 (comment) but then I noticed there was a few other ones.

@youknowriad
Copy link
Contributor

Prevent fields that are part of a combined field from being displayed when they are listed in view.fields

I feel we should be able to show the fields individually in separate columns if we choose to.

: enableHiding,
};
} );
if ( view.type === LAYOUT_TABLE && view.layout?.combinedFields ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this works for now, but my thinking is that ultimately, we should find a way for the layouts to actually "inject" logic here. Or (which is what I had in mind) to have some kind of a slot and the "properties" area be specific per layout. For instance in "table", we should be able to "combine" fields if we want.

Copy link
Member Author

@oandregal oandregal Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. If we aim to make the layouts "registerable/unregisterable" by 3rd parties we cannot have this kind of logic in the view config (or any other component).

I've looked at the current usage of layouts in this component and this is what I see:

All of them could be solved by providing a layout.supports thing somewhere. Using that approach, it'd still be DataViews' responsibility to define which "features" are supported.

Or (which is what I had in mind) to have some kind of a slot and the "properties" area be specific per layout. For instance in "table", we should be able to "combine" fields if we want.

This approach would be a lot more open. However, every layout would have to implement the same actions (reordering, visibility, etc.).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any case, not for this PR 😅

Copy link
Member Author

@oandregal oandregal Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, layout.supports could also be a nice thing to have for consumers that want to "lock in" (aka remove options) a bit more the user experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach would be a lot more open. However, every layout would have to implement the same actions (reordering, visibility, etc.).

Yes, but for list view we have badges..., for grid we have columns and rows...

@oandregal
Copy link
Member Author

Prevent fields that are part of a combined field from being displayed when they are listed in view.fields

I feel we should be able to show the fields individually in separate columns if we choose to.

I could be convinced otherwise, but I can't imagine use cases for this.

Anyway, if we wanted this, we should consider it at the user level (expose them in the view config under "hidden fields"). A feature that is related is the ability for users to "combine"/"split" fields. Not sure how both things would work together, so I'd rather leave this out of this PR.

@youknowriad
Copy link
Contributor

I could be convinced otherwise, but I can't imagine use cases for this.

I agree that if we allow people to combine and split combined fields, it becomes kind of useless. I'm fine keeping the current state of the PR.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have thoroughly tested the reordering and hiding functionality of patterns, templates, and pages view, and everything seems to be working well. I didn't encounter any unexpected issues with the code changes, so it looks like we can go ahead and merge this change. 👍

@oandregal oandregal merged commit 7c0638b into trunk Sep 3, 2024
77 checks passed
@oandregal oandregal deleted the fix/dataviews-field-reordering-and-visibility branch September 3, 2024 14:18
@github-actions github-actions bot added this to the Gutenberg 19.2 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants