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

feat: conditional tabs #8406

Closed
wants to merge 12 commits into from

Conversation

willviles
Copy link
Contributor

@willviles willviles commented Sep 25, 2024

This is a POC for conditional tabs as per #1840.

Example

payload-conditional-tabs.mov

Learnings

I thought it'd be an easy win, as it seems quite a simple client-side feature. But from implementing this POC, I learned that:

  • Conditional field rendering is controlled by a passesCondition prop, which is held in form state server side
  • This is because admin.condition functions can't be serialised to client form components
  • Tab fields themselves have no form state

Proposal

With the learnings in mind, my implementation:

  • Requires any tab fields with conditional tabs to be given an id property
  • This enables form state to be set for the individual tabs via an indexed path like ${id}.${i}
  • This reuses the existing conventions for passesCondition, rather than introducing more code uniquely for the tab field

Here's how a tab field with conditional tabs would now error if an admin condition were passed to it:

{
  type: 'tabs',
  tabs: [
    {
      label: 'Conditional Tab',
      description: 'This tab should only be visible when the conditional field is checked.',
      fields: [/** ... */],
      /** 
        TS Error: Object literal may only specify known properties, 
        and 'admin' does not exist in type 'TabWithoutCondition' 
      */
      admin: {
        condition: ({ conditionalTabVisible }) => !!conditionalTabVisible,
        
      },
    },
  ]
}

But if an id is provided, then it's all gravy:

{
  type: 'tabs',
  /** Adding an ID satisfies the type */
  id: 'conditional-tabs',
  tabs: [
    {
      label: 'Incorrect Conditional Tab',
      description: 'This tab should only be visible when the conditional field is checked.',
      fields: [/** ... */],
      admin: {
        condition: ({ conditionalTabVisible }) => !!conditionalTabVisible,
      },
    },
  ]
}

Further Notes

As per my implementation:

  • Form state is only set for tabs with a condition present. The form state is the most minimal object possible as per valid fieldState, similar to other 'non state affecting' fields such as the ui field.
  • A runtime error is thrown for any use of admin.condition without giving the tabs field an id
  • An error is thrown during collection config sanitization if any tab fields have been assigned the same id.
  • The TabField client component keeps track of all of its tabs' visibility via useFormFields and is smart enough to shift the active index to a visible tab if the current tab is hidden.
  • Using a property on the tab field called id is unique to Payload's field config, however I would suggest that using either name or label would be confusing DX. As a long-time Payload user, name indicates to me that a field is tied to a collection's data and label is a visible string displayed within the UI.

@willviles willviles force-pushed the feat/conditional-tabs branch from a90abfd to 31463e4 Compare September 25, 2024 14:39
@jmikrut
Copy link
Member

jmikrut commented Sep 26, 2024

Yeesssssssss

Thank you! We are going to review this as soon as possible!

Copy link
Contributor

@DanRibbens DanRibbens left a comment

Choose a reason for hiding this comment

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

Wonderful PR, I pulled it down and it works great!
I just have one simplification idea that we should try and handle instead of relying on the config.

@@ -267,6 +268,11 @@ export const sanitizeFields = async ({
if (field.type === 'tabs') {
for (let j = 0; j < field.tabs.length; j++) {
const tab = field.tabs[j]

if ('admin' in tab && tab.admin?.condition && !field.id) {
throw new MissingTabsId()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just generate some kind of id for them? I don't think you should have to make your own id in your config to use this feature. What do you think?

@paulpopus
Copy link
Contributor

Just to update you here, reviewing again and I'll make a commit against that ^ ID change, there's a way to do the form-state for a field even without a set name or ID, similar to collapsible fields

@paulpopus
Copy link
Contributor

paulpopus commented Oct 15, 2024

Weird, I thought gh would let me push to your PR directly but I guess not #8720

Your commits are still saved in here though, you can cherry pick my commits into here, or we can merge that one and you should get co-authorship. Whichever way you prefer

When adding in the field index-based field state, we found a regression where collapsible fields were not correctly mapping either, so my commits there will fix that as well

@willviles
Copy link
Contributor Author

Awesome @paulpopus. Sorry for the delay in response my side.

@DanRibbens, I introduced the id property because I wasn't sure whether index-based field state would be suitable. My main concern being that if open states were to be stored either in the db via user preferences or in localstorage, changes to the Payload config could alter the desired state. However, if that's not a use case, then I'm delighted to see the API is simplified.

I've checked out #8720 and it works nicely. Thanks guys!

@paulpopus
Copy link
Contributor

Alright, gonna close this one then in favour of the other one, thanks again!

@paulpopus paulpopus closed this Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants