payload icon indicating copy to clipboard operation
payload copied to clipboard

feat: optional tab namespacing

Open slavanossar opened this issue 3 years ago • 1 comments

Description

This adds the ability to optionally provide a name value to a tab, and have it function the same as a Group when storing/retrieving from the database.

As an example, the following collection:

const collection = {
  slug: 'tab-examples',
  fields: [
    {
      type: 'tabs',
      tabs: [
        {
          label: 'First tab'
          fields: [
            {
              name: 'text'
              label: 'Text'
            }
          ]
        },
        {
          name: 'myTab',
          label: 'Second tab'
          fields: [
            {
              name: 'text'
              label: 'Text'
            }
          ]
        },
      ]
    }
  ]
}

Would return this data:

{
  "myTab": {
    "text": "This is exampled text in a named tab."
  },
  "text": "This is example text"
}
  • [x] I have read and understand the CONTRIBUTING.md document in this repository

Type of change

  • [x] New feature (non-breaking change which adds functionality)
  • [x] 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
  • [x] I have made corresponding changes to the documentation

slavanossar avatar Jul 25 '22 06:07 slavanossar

I had a couple of questions before I mark it ready for review:

  1. I'm having issues with displaying the values in the example array within Tab with Name. I updated how paths are defined for tabs (here and here), and the name attribute of the inputs reflects this with tab.array.X.text. Also updating and saving the field correctly updates the data in the database (confirmed via GraphQL response), but on initial render the text values in tab.array use the values from array. The other text field in the tab displays the correct value, so I feel like I've missed something – any ideas?
  2. I'm not very familiar with test suites, but I couldn't see anything that was testing the Tabs field implementation (maybe because they were only presentational?). Is this something that needs to be added?

This is my first time contributing to a repo this complex, so all feedback is welcome!

slavanossar avatar Jul 25 '22 07:07 slavanossar

@slavanossar I pushed some updates to your branch, good work overall. Some of the changes I covered were:

  • named tabs make the label optional
  • unnamed tabs cannot be localized
  • typescript types and schema validation for tabs is more accurate regarding above changes
  • tabs cannot be localized at the main level
  • added tests

There seems to be an issue with default values inside of the tabs still. If you have time to look at it, let us know, otherwise I'll pick it back up when I get time.

We also need some graphql tests coverage.

More manual testing should be done before merging, aside from just the automated testing mentioned.

DanRibbens avatar Aug 05 '22 13:08 DanRibbens

Hey @slavanossar — thanks again for the work on this branch. You really dove in there and got a lot done and this feature adds a lot of value.

The team and I went through everything this evening and wrapped it up. All tests pass, features are built, and everything's working great!

THANK YOU!

jmikrut avatar Sep 12 '22 04:09 jmikrut

Sorry I completely missed Dan's response, been pretty busy the past month with work.

Thanks for completing it, will go through and have a read through the additional changes so I can understand it better 👍

slavanossar avatar Sep 12 '22 04:09 slavanossar