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

Adding 'object' type as a datatype option #62

Closed
wants to merge 7 commits into from

Conversation

Danm72
Copy link

@Danm72 Danm72 commented Sep 10, 2023

Which can be used to embed objects within the sidebar to embed other editor objects within blocks or to create more details objects.

In this case you can see the 'component' Heading is used to supply object fields to the Block Hero, the values are then used to actually style the heading object within the hero.

demo.mp4

#60

@vercel
Copy link

vercel bot commented Sep 10, 2023

@Danm72 is attempting to deploy a commit to the Measured Team on Vercel.

A member of the Team first needs to authorize it.

@chrisvxd chrisvxd marked this pull request as draft September 10, 2023 14:25
@vercel
Copy link

vercel bot commented Sep 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
puck-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 14, 2023 9:58am

Copy link
Member

@chrisvxd chrisvxd left a comment

Choose a reason for hiding this comment

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

Thanks for the issue and WIP PR @Danm72! Great work!

I'm think I'm in favour of adding support for object params (because we have array), and can certainly think of some use-cases where a flat model doesn't cut it. Styling inner components is likely just one use case of many (and one that will probably become less important with #37, which will allow for nested components).

A few things come to mind when looking through your PR and thinking about this feature:

  1. I'm not a huge fan of the array UI in general, and not totally sure if it makes sense for object fields. It's totally fine to reuse as part of this initial implementation, but we might want to revisit it at some point. Which brings me onto...
  2. Inline fieldsets at the top-level (with headings) are something I've considered as a way to break the form up into separate areas of related fields. It strikes me that the Heading scenario here might also work with an inline Heading fieldset. Again, not for this PR but I think worth the discussion point.
  3. And finally, the only thing I think we should perhaps reconsider is whether to include the Heading fields in the Hero for the demo component. Whilst it shows the functionality well, I worry it bloats the demo out and isn't something need to necessary know about from first impressions.

PS how are you producing those lovely screen recordings? 😍

@Danm72
Copy link
Author

Danm72 commented Sep 10, 2023

Awesome feedback I'll respond when I'm back at my PC but all sounds logical to me!

I agree about the array UI and might noodle about with the fieldset concept!

Good point on the demo.

Videos are made with the awesome https://www.screen.studio

@chrisvxd
Copy link
Member

Great! And no rush.

I've made a separate ticket to discuss a fieldset API - #64

@Danm72 Danm72 changed the title [WIP] Creating an 'object' type Adding 'object' type as a datatype option Sep 10, 2023
@Danm72
Copy link
Author

Danm72 commented Sep 10, 2023

Hey @chrisvxd added those small changes lemme know if you see anything else. Feel free to go in another direction with this!

@Danm72 Danm72 closed this Sep 13, 2023
@chrisvxd
Copy link
Member

Hey @Danm72! Just checking why you closed this?

Was going to review as soon as I had time. Happy with general direction

@Danm72
Copy link
Author

Danm72 commented Sep 13, 2023

Hey Chris,

Whoops sorry I thought it was going stagnant and didn't want to leave it sitting there!

Feel free to merge it or run with it I was just thinking your other PR around field rendering might be a little more advanced with the 'custom' type.

Maybe both flows could work. Change my PR to use type 'custom' instead of object and the default custom renderer is the array renderer?

Unsure if id go with object or custom..

@chrisvxd
Copy link
Member

Hi @Danm72! Been introspecting on this a bit and think we're going to close it out per your suggestion until after we've looked fieldsets and tabs in #64. Thanks for all your work :)

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.

2 participants