Skip to content

feat: hide fields in version view + admin.hiddenInVersionView #7724

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

franciscolourenco
Copy link
Contributor

@franciscolourenco franciscolourenco commented Aug 16, 2024

Description

  • Hides fields that have hidden and admin.disabled from the version view.
  • Adds a new option to hide a field specifically from the version view: admin.hiddenInVersionView.

I've considered other names for the new option: admin.disabledInVersionView; admin.disableVersionView. admin.disableInVersionView follows the pattern of admin.disableListColumn and admin.disableListFiler. However, it could be confused with a disabled state of an input, so I ended up going with hiddenInVersionView because it is more precise, and follows the pattern of admin.hidden, but I'm open to other ideas and preferences on the naming.

Speaking of admin.hidden, we should probably consider renaming this option to something like admin.hiddenInEditView, since it can be easily confused with the hidden option that most fields also have. The first, only hides a field from the edit view, while the later hides a field everywhere, including the API.

  • I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have added tests that prove my fix is effective or that my feature works
  • Existing test suite passes locally with my changes
  • I have made corresponding changes to the documentation

@franciscolourenco
Copy link
Contributor Author

@zubricks any chance of getting some feedback on this one? We are going to need it in v3 as well.

@jacobsfletch
Copy link
Member

@franciscolourenco I'm wondering if admin.hidden should accept a boolean or an object:

admin: {
  hidden: {
    versions: true
  }
}

Speaking of admin.hidden, we should probably consider renaming this option to something like admin.hiddenInEditView, since it can be easily confused with the hidden option that most fields also have.

The keyword admin here is what we expect to be the differentiator, where hidden: true hides that field from API responses entirely, but admin.hidden is solely UI.

@franciscolourenco
Copy link
Contributor Author

franciscolourenco commented Dec 2, 2024

@jacobsfletch I think that would be a pretty good API, but.. wouldn't it be sort of a breaking change, or at least hard to explain why hidden: true means hidden in edit view, instead of hidden in all views? Am I understanding the purpose of admin.hidden correctly?

This is the current state of the API afaict:

hidden: true // hidden everywhere including APIs
admin: {
  hidden: true, // hidden in edit view (and bulk edit)
  disabled: true, // hidden in all views
  disableBulkEdit: true, // hidden in bulk edit select
  disableListColumn: true, // hidden in the list view table
  disableListFilter: true, // hidden in the filters
}

I settled with hiddenInVersionView because I wanted to avoid the double meaning that disableVersionView can have (confusion with readOnly). However I didn't realize that there is a admin.disabled option which already suffers from this ambiguity.

Any other ideas?

@jacobsfletch
Copy link
Member

jacobsfletch commented Dec 2, 2024

@jacobsfletch I think that would be a pretty good API, but.. wouldn't it be sort of a breaking change, or at least hard to explain why hidden: true means hidden in edit view, instead of hidden in all views? Am I understanding the purpose of admin.hidden correctly?

Yea this would be a breaking change, but should definitely implement it in v4. If you set admin.hidden: true outright, it would hide it from all views. But if you wanted to have more control, you'd set this to an object instead, which will allow you to target only a subset of those views. So you could do either of the following:

admin: {
  hidden: true // hides this field from _all_ views in the admin panel
}
admin: {
  hidden: {
    field: true, // hides this field from the edit view only
    filter: true, // hides from list view filter controls
    column: true, // hides from list view column selector
    diff: true // hides the diff from the versions view
  }
}

However I didn't realize that there is a admin.disabled option which already suffers from this ambiguity.

Yep, this is pretty much the same scenario. IMO a boolean or object would also have been a better choice for admin.disabled as well. If anything, I think that should be changed in the future. This way naming properties is easier, and these properties are contextual amongst each other.

@franciscolourenco
Copy link
Contributor Author

If you set admin.hidden: true outright, it would hide it from all views, just as it does now.

This doesn't seem to be the case now, which I found unexpected, and why I suggested renaming it to hiddenInEditView (or
disableEditView). Currently with admin.hidden:true:

Edit view: hidden
Bulk operations: hidden
List view columns: shown
List view filters: shown
Versions view: shown

Unless this is a bug, and the intention was for admin.hidden to hide the field everywhere, even though the docs don't suggest this. If admin.hidden hid the field everywhere, then it would effectively be the same as admin.disabled?

@denolfe denolfe added the v2 label Dec 3, 2024
@franciscolourenco franciscolourenco changed the title feat(payload): ability to hide fields from the version view with admin.hiddenInVersionView feat: hide fields in version view + admin.hiddenInVersionView Jan 15, 2025
@franciscolourenco franciscolourenco force-pushed the allow-fields-to-be-hidden-in-version-view branch from 2dd7b56 to da3aa28 Compare January 15, 2025 09:28
@franciscolourenco franciscolourenco changed the base branch from 2.x to main January 15, 2025 09:28
@franciscolourenco
Copy link
Contributor Author

@jacobsfletch I've ported the changes to v3 and force pushed to this branch, so the v2 label can be removed.

This is ready to review from my point of view. To consider:

  1. The new behavior seems more correct and does what the hidden and admin.disabled options suggest, but is different from the way it worked until now.
  2. Are we ok with admin.hiddenInVersionVerion or do we prefer admin.disableVersionView or other?

@franciscolourenco franciscolourenco force-pushed the allow-fields-to-be-hidden-in-version-view branch from da3aa28 to 6ad1126 Compare January 15, 2025 10:48
@franciscolourenco franciscolourenco force-pushed the allow-fields-to-be-hidden-in-version-view branch from 6ad1126 to 9ad5c44 Compare January 23, 2025 05:28
BREAKING CHANGE: some users might have come to expect these fields to
be displayed in the version view, but it doesn't seem to make sense
@franciscolourenco franciscolourenco force-pushed the allow-fields-to-be-hidden-in-version-view branch 2 times, most recently from fe539aa to 48f493b Compare January 26, 2025 13:39
@franciscolourenco
Copy link
Contributor Author

@jacobsfletch @denolfe

I've updated the PR to take into consideration the changes introduced by #8054 and it is ready to be reviewed.

If you are concerned about hiding field.hidden and field.admin.disabled fields from the Version view, I could easily remove that, since having field.admin.hiddenInVersionView would already allow us to control the visibility.

@franciscolourenco franciscolourenco changed the title feat: hide fields in version view + admin.hiddenInVersionView feat: hide fields in version view + admin.disableVersionView Mar 3, 2025
@franciscolourenco franciscolourenco force-pushed the allow-fields-to-be-hidden-in-version-view branch 2 times, most recently from fcf639f to 4544268 Compare March 3, 2025 13:13
@franciscolourenco franciscolourenco changed the title feat: hide fields in version view + admin.disableVersionView feat: hide fields in version view + admin.hiddenInVersionView Mar 3, 2025
@JarrodMFlesch JarrodMFlesch removed the v2 label Mar 18, 2025
@JarrodMFlesch
Copy link
Contributor

@franciscolourenco I am confused as to why we need a specific variable for the versions view. I feel as if a field has admin.hidden that it should be hidden in the versions view as well. If that is not how it currently works, which is what it sounds like, I would consider it a bug.

@jacobsfletch wdyt?

@philipp-tailor
Copy link
Contributor

@JarrodMFlesch I think the point of admin.hiddenInVersionView is to hide fields in the version view that are not hidden in other views. People browsing versions sometimes find some metadata to be "confusing noise" instead of it helping them to understand how 1 version is different from another. So being able to hide fields with this new configuration would be sweet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants