measuredco / puck

The visual editor for React
https://puckeditor.com
MIT License
5.08k stars 285 forks source link

Group components in the sidebar #87

Closed chrisvxd closed 11 months ago

chrisvxd commented 1 year ago

A developer should be able to group components in the left hand sidebar.

Proposal

Option 1

Add the tags array to the component config object.

Additionally, this can be used restrict which components can be dropped into a DropZone (see #37)

Option 2

As option 1, but singular tag.

Option 3

A render function may provide similar functionality by allowing the developer to the override the UI. Similar to the fieldset proposal #64.

xaviemirmon commented 11 months ago

Hey @chrisvxd

I've created a PR for this feature following option 2. https://github.com/measuredco/puck/pull/138

This is on the basis that if you tag a component with multiple tags, you'd then have to have multiple droppable instances of the component which doesn't make sense to me.

I've also built this functionality in such a way that it renders the same as before so if you don't have any labels the behviour is the same.

chrisvxd commented 11 months ago

Thanks @xaviemirmon. I think your rationale behind avoiding option 1 is interesting.

I think tags extend well to #103, which can take advantage of the tagging to specify which components can be dropped:

<DropZone zone="content" tags=["layout"] />

(You could also use component names like <DropZone zone="content" components=["HeadingBlock"] />)

Option 3 is perhaps closer to the proposal for #64 for fieldsets, and what we've implemented in #58 for custom fields. I imagine you would add a new prop called renderComponentList (or something more pithy) to Config


renderComponentList: () => {
  return <ComponentList>
    <ComponentList.Group title="Layout">
      <ComponentList.Item component="Section" />
      <ComponentList.Item component="Columns" />
    <ComponentList.Group />

    <ComponentList.Group title="Typography">
      <ComponentList.Item component="Heading" />
      <ComponentList.Item component="Paragraph" />
    <ComponentList.Group />
  </ComponentList>
}

This might be too verbose, but also provides more flexibility for the user. The APIs also aren't mutually exclusive.

Interestingly, this option could mean we still reserve the use of tags (plural) on the ComponentConfig for the purposes of DropZones.

xaviemirmon commented 11 months ago

@chrisvxd I agree and think the tags option for DropZone makes sense, and I have some thoughts on that, which I'll leave on the other issue.

Thanks for outlining option three a bit more. I've got my head around the proposal now.

I feel this option is too verbose and possibly prone to errors. What would happen if someone edited their component schema and removed or renamed a component? It looks like you would then have to remove it from the renderComponentList: also, or your instance would error. This approach makes sense for fieldsets where the shape of the data is quite variable but I personally am leaning towards option 2.

JakeSidSmith commented 11 months ago

A nested config would simplify the grouping process (for dev using puck at least), but make puck a bit harder to maintain (as you need to check if it is a group or a component, and the types might get a little insane).

components: {
  'Group name': {
    Button: {...etc}
  }
}
ahmedrowaihi-official commented 11 months ago

A nested config would simplify the grouping process (for dev using puck at least), but make puck a bit harder to maintain (as you need to check if it is a group or a component, and the types might get a little insane).


components: {

  'Group name': {

    Button: {...etc}

  }

}

It looks nice, but this way, will change how the renderer as well,

Your solution is good, But I vote for adding tag optionally to each component's config

So components with same tags will be grouped on the side bar, and the renderer will work as is with no changes

xaviemirmon commented 11 months ago

@ahmedrowaihi-official That was the approach I took with PR #138 e.g. https://github.com/measuredco/puck/pull/138/files#diff-2b1e1478c2c87c923e201dbb6af3a48045e0f0a3114c93c8837edbbce3b8e3c7 Component definition has a "tag" field which is an optional part to the component config definition.

@JakeSidSmith I'm curious, what would be the benefit of nesting the definition inside a tag? I feel like this would force you to tag your components which might not be desired?

JakeSidSmith commented 11 months ago

Hey, @xaviemirmon thanks for that link. This is only the second time I've heard someone mention "tags" and I had no idea that it was a config option - I thought they were talking about HTML tags. As a side note: I would recommend naming that something else to avoid confusion. Tags (besides also being an HTML term) can normally have multiple values whereas things normally only have a single "category" or "group" (category would be my preference).

To answer your question about "what would be the benefit of nesting the definition inside a tag" - I'm not suggesting this be inside a tag, but instead be a nested config e.g. this could become:

export const conf: Config<Props, RootProps> = {
  root: {
    render: Root,
  },
  components: {
    Layout: {
      Hero,
      Columns,
      Flex,
      VerticalSpace,
    },
    Widget: {
      Card,
      Stats,
    },
    Image: {
      Logos,
    },
    Form: {
      ButtonGroup,
    },
    Text: {
      Heading,
      Text,
    }
  },
};

Keeping this config in one place makes it easer to keep the groupings in sync. If you misspell a group here it only has the effect of showing a bad group name in the UI, whereas if you misspell a tag in one of your components, it'll end up in a different group or not grouped at all. Similarly if I want to change the name of the group there's only a single piece of text I have to change rather than having to look through all of my component files and update the ones that are in the group I want to rename.

I guess what I'm saying is: this is an alternative to the tags approach, where all config is in a central location.

That being said, I can also see the benefits of storing component related data with that component e.g. you could dynamically create a puck config by looking at all the modules in a components directory.

ahmedrowaihi commented 11 months ago

@JakeSidSmith Well, I think the term tag is creating misrepresentation ๐Ÿ˜…, I am unconfidently suggesting we change it to category it might be more representative

Regarding your approach, I see your point

components:{
   Images:{
     ImageComponent,
     LogosComponent
   }
}

This will change the "mapping" behaviour in both renderer, and the side bar editor

This will even allow to have as much as wanted nesting

meanwhile, for the option with

components:{
     ImageComponent:{
     tag:"images"
     }
     LogosComponent:{
     tag:"images"
     }
}

It will be a bit restrictive, but require changing the "mapping" behaviour only for the side-bar since the renderer will have nothing to take care about tags

it's like a trade-off๐Ÿ˜…

xaviemirmon commented 11 months ago

@JakeSidSmith

Thanks for explaining that it makes sense now. I agree that tag should be changed to group or category. I have a couple of uncertainties with the approach outlined above.

  1. Components in multiple groups. I want help seeing how you'd limit to one group or instance. I could see this allowing too much flexibility e.g. see the different instances of hero.
  components: {
    Layout: {
      **Hero,**
      Columns,
      **Hero,**
      Flex,
      **Hero,**
      VerticalSpace,
    },
    Widget: {
        **Hero,**
    },
    Image: {
        **Hero,**
    },
    ...
  },

Having the same component in multiple groups/categories would allow for the possibility of the editorial experience in my view, becoming too complex.

  1. Backward compatibility. This change in the shape of the data would introduce a breaking change, and @ahmedrowaihi has suggested the need for refactoring outside of the sidebar.

How about the following?:

component: {
  ComponentName,
  ...
},
category: {
  default: {
    label: "Others"
  }, 
  layout: {
    label: "Layout"
  }
}

Then in the component config:

{
  fields: {},
  defaultProps: {},
  category: "layout",
  render: () => {
    return ...
  },
};

Category could be optional, so if you don't wish to categorise your components, you don't have to and the sidebar would behave as it does now.

What do you all think?

JakeSidSmith commented 11 months ago

In the below example TypeScript would complain about multiple instances of the same component being within the same group ("An object literal cannot have multiple properties with the same name."), and I don't think allowing components to appear in multiple groups is a bad idea. There are times when this makes sense e.g. if you had groups for both "form" and "navbar" a button might appear in both - it just helps the end user logically group their choices. They shouldn't be different instances of that component unless they are a different component (unless I'm misunderstanding how puck determines component equality).

  components: {
    Layout: {
      **Hero,**
      Columns,
      **Hero,**
      Flex,
      **Hero,**
      VerticalSpace,
    },
    Widget: {
        **Hero,**
    },
    Image: {
        **Hero,**
    },
    ...
  },

I also don't think this would be a breaking change. The existing interface would continue to work as is, and you have the option to group components. You could even have both groups and ungrouped components if you like. E.g.

components: {
  Forms: {
    Button,
  },
  Paragraph,
}

I actually quite like your alternative approach above. Having type-safe identifiers for the groups and a separate name would allow people to keep their component categories in sync with the config.

I'm not totally sure about the config though. I would avoid having a "default" group as people might not want ungrouped components to appear in a group. I think the definition of the category names (and whatever else we might want to put in there) should be kept separate from the sidebar layout, as they aren't necessarily related. Early thoughts:

{
  categories: {
    layout: {
      label: "Layout"
    },
    form: {
      label: "Form"
    }
  },
  sidebarCategories: ['form', 'layout']
}
xaviemirmon commented 11 months ago

@JakeSidSmith Right, it's making sense now. My only remaining concern with the approach above is a codebase that needs to use TS, or you will encounter problems.

@chrisvxd How do you feel about this approach outlined above? Are you happy with TS being a requirement to launch a new site?

If we're all good, shall I update my PR with the changes listed above?

xaviemirmon commented 11 months ago

Also, as an aside does anyone have suggestions on how to alter the cancel drop behaviours?

JakeSidSmith commented 11 months ago

My only remaining concern with the approach above is a codebase that needs to use TS, or you will encounter problems.

I think that's just kinda the case with JS vs TS in general. ๐Ÿ˜‚ TypeScript wouldn't be required, but would help you catch errors at compile time.

For those using JS, there are ways to prevent these things as well e.g. https://eslint.org/docs/latest/rules/no-dupe-keys

JakeSidSmith commented 11 months ago

I'm actually leaning towards the idea of having the categories defined per component, and a central place to define additional category data e.g. names, and sidebar layout.

I think the sidebar layout could be much more powerful with something like @xaviemirmon approach.

And I'm pretty sure I could make the types strict enough that I could prevent components accidentally having the wrong category identifiers (for people like me who like everything strongly typed). But, again, this could be optional.

Another thing to note is that it would be best to keep the categories/tags separate from the sidebar layout, as you may have, for example:

Forms: 
  Form
  TextInput
  NumberInput
  Button

Where you'd like all of these components to appear within a "Forms" group, but you would want to restrict which components can be dropped into a Form component. All the inputs and buttons are fine, but you probably want to disallow dropping another form inside a form.

ahmedrowaihi commented 11 months ago

This is amazing

chrisvxd commented 11 months ago

Sorry all, busy week for me. I haven't had time to engage with / process this and I'm now away for a few days.

I'll check back mid-next week!

chrisvxd commented 11 months ago

I agree with you all that tags is confusing and we should go with category ("group" is too vague). The rest, I'm unclear on. Seems like we've discussed a lot of options and I'm not clear where the momentum is, so I'm going to try to summarise them here and propose some further options.

@xaviemirmon I think you should still hold off because it doesn't seem clear yet.

TLDR I'm leaning towards option 4.

Option 1 - category key

  categories: {
    layout: {
      name: "Layout"
    },
    actions: {
      name: "Actions"
    }
  },
  components: {
    Columns: {
      ...Columns,
      category: "layout"
    },
    Flex: {
      ...Flex,
      category: "layout"
    },
    Button: {
      ...Button,
      category: "actions"
    }
  },

Option 1b

This works but it restricts 1 component to 1 category, a restriction that I agree with @JakeSidSmith seems artificial. Instead, we could use categories:

  categories: {
    layout: {
      name: "Layout"
    },
    actions: {
      name: "Actions"
    }
  },
  components: {
    Columns: {
      ...Columns,
      categories: ["layout"]
    },
    Flex: {
      ...Flex,
      categories: ["layout"]
    },
    Button: {
      ...Button,
      categories: ["actions"]
    }
  },

To implement category restrictions in DropZones, we would probably need a helper:

const getComponentsInCategory = (category: string) =>
  Object.keys(
    config.components.find((component) => component.category === category)
  );

const MyComponent = () => (
  <>
    <DropZone restrict={getComponentsInCategory("actions")} />
    <DropZone restrict={["Button"]} />
  </>
);

Alternatively you could add an additional API, restrictCategories, but this might be duplicative

const MyComponent = () => (
  <>
    <DropZone restrict={["Button"]} />
    <DropZone restrictCategories={["actions"]} />
  </>
);

I find this the easiest to get my head around as it doesn't require messaging with the existing types or any complicated lookups.

Option 2 - conditional types

  categories: {
    layout: {
      name: "Layout"
    },
    actions: {
      name: "Actions"
    }
  },
  components: {
    layout: {
      Columns,
      Flex,
    },
    actions: {
      Button
    },
    Hero // no category
  },

This would allow you to make some interesting simplifications in the #103 proposal:

const MyComponent = () => (
  <>
    {/* if accepting array of strings */}
    <DropZone restrict={Object.keys(config.components.actions)} />
    <DropZone restrict={["Button"]} />

    {/* if accepting object */}
    <DropZone restrict={config.components.actions} />
    <DropZone restrict={{ Button: config.components.actions.Button }} />
  </>
);

Some concerns with this approach:

Option 3 - blended

Option 3a

  components: {
    layout: {
      name: "Layout"
      components: {
        Columns,
        Flex,
      },
    },
    actions: {
      name: "Actions"
      components: {
        Button
      }
    },
    Hero // no category
  },

Option 3b

You could also keep the legacy components API untouched and add a new categories API

  categories: {
    layout: {
      name: "Layout"
      components: {
        Columns,
        Flex,
      },
    },
    actions: {
      name: "Actions"
      components: {
        Button
      }
    },
  },
  components: {
    Hero
  }

Option 4 - categories API only

  categories: {
    layout: {
      name: "Layout"
      components: ['Columns', "Flex"]
    },
    actions: {
      name: "Actions"
      components: ['Button']
    },
  },
  components: {
    Columns,
    Flex,
    Button,
  }

Restricting DropZone

const MyComponent = () => (
  <>
    <DropZone restrict={config.categories.actions.components} />
  </>
);

Option 5 - Render function

import { Puck, ComponentList } from "@measured/puck";

const Editor = () => (
  <>
    <Puck
      config={{
        components: {
          Columns,
          Flex,
          Button,
        },
      }}
      renderComponentList={() => {
        return (
          <>
            <ComponentList.Section title="Layout">
              <ComponentList.Item component="Columns" />
              <ComponentList.Item component="Flex" />
            </ComponentList.Section>

            <ComponentList.Section title="Actions">
              <ComponentList.Item component="Button" />
            </ComponentList.Section>
          </>
        );
      }}
    />
  </>
);
const MyComponent = () => (
  <>
    <DropZone restrict={["Button"]} />
  </>
);

The user could still create categories in their own fashion:

import { Puck, ComponentList } from "@measured/puck";

const categories = {
  layout: {
    title: "Layout",
    components: ["Columns", "Flex"],
  },
  actions: {
    title: "Actions",
    components: ["Button"],
  },
};

const Editor = () => (
  <>
    <Puck
      config={{
        components: {
          Columns,
          Flex,
          Button,
        },
      }}
      renderComponentList={() => (
        <>
          {Object.keys(categories).map((category) => (
            <ComponentList.Section title={category.title}>
              {category.components.map((componentName) => (
                <ComponentList.Item component={componentName} />
              ))}
            </ComponentList.Section>
          ))}
        </>
      )}
    />
  </>
);

const MyComponent = () => (
  <>
    <DropZone restrict={categories.actions} />
  </>
);

Note, this is a similar data model to Option 4 and may be a natural extension to it, or we can do it first and defer a decision on a final category API.


Personally, I'm leaning to option 4 with 5 as a follow-on. Category data seems like a separate concern to the component configuration, and I think keeping it separate makes sense.

Have I missed anything? Any thoughts or preferences?

JakeSidSmith commented 11 months ago

I am a fan of 1b - categories defined per component, and each can have multiple categories. This seems to be the most powerful when you consider both grouping display and dropping restrictions.

In the below example I can display all my form components in a "form" group, and easily restrict which components can be dropped inside a form.

Form: {
  categories: ['form']
}

Input: {
  categories: ['form', 'formContent']
}
Button: {
  categories: ['form', 'formContent']
}

For this reason I would urge that the category names (and other info) are defined separately from the sidebar layout, as it may be necessary to define some property of a category that you don't want displayed in a separate group in the sidebar (e.g. formContent).

I'm a fan of the restrictions being handled by additional props, but I would go with:

<DropZone
  allowComponents={[]}
  disallowComponents={[]}
  allowCategories={[]}
  disallowCategories={[]}
/>

Having each components/categories separated as well as separating the allow/disallow will allow (using that word way too much now) us to strongly type the restrictions based on the puck config. This could be done with a couple of restrictComponents and restrictCategories, but if this uses a !component syntax, the types will be a bit more messy. Personally I think it'd also be easier for the user to provide the component/category names as is (without any prefixes).

xaviemirmon commented 11 months ago

I'm with @JakeSidSmith on this one. 1b is the preferred option for me.

For my use case, I'd only use one tag, but this would also cater to that. What I like about this option is that it opens up categories as an API to other parts of Puck in future. So if, say, there was a media API created, we could use this approach there as well.

I also like @JakeSidSmith's approach to passing the dropzone criteria as props.

chrisvxd commented 11 months ago

I had further offline discussions on this with Measured team and @xaviemirmon, and did some code experiments via #183, testing both option 1b and option 4. I went with option 4 for reasons outlined in the PR.

@JakeSidSmith Moving remaining DropZone-related conversation over to #103

Thanks all for your thorough engagement here ๐Ÿ™

chrisvxd commented 11 months ago

Available in 0.11.0-canary.c8c02fd ๐Ÿ™Œ