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(object): variable types #7031

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

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Jan 12, 2024

Summary

Allow for objects to use variable types - effectively enabling "dependent fields" for objects. Also, improve support for objects with required: false - use list widget for those too, so that you can add remove the whole object from the CMS, depending on whether the content author wants it to be defined or not.

Implementation: use the list widget control for objects when either types is defined, or when required=false.

Replaces #3891
Goes some of the way to addressing #565 (there's a limitation - dependent fields must be nested under an object or list).

Test plan

Added the typed_object field to the "kitchen sink" collection and tried it out.

Here's how it renders:

Screen.Recording.2024-01-12.at.11.19.25.AM.mov

Help needed

Code organisation: I don't like that the object widget is now actually defined in the decap-cms-widget-list package. The decap-cms-widget-object package is now reduced to a dependency of the list widget. To be honest I don't really see the value in splitting the widgets out into separate packages anyway, but that's another story. It was a minimal way of getting the change in. Maybe a refactor to merge the decap-cms-widget-list and decap-cms-widget-object packages could be a follow-up.

Naming: similarly, the helper which creates the widget is called DecapCmsWidgetList.ObjectWidget() but that's kind of confusing. It's the object widget. So maybe the following refactor would make sense:

  1. Rename the decap-cms-widget-object package to decap-cms-widget-struct - it's just a general purpose control component for rendering the fields for an dictionary-like field (either object or list). It doesn't correspond to an actual widget anymore
  2. Create a new package called decap-cms-widget-object-and-list which depends on both decap-cms-widget-list and decap-cms-widget-struct. This new package is just a thin wrapper which decides whether to use the list control or the struct control.

Testing: I tested it manually, but noticed there's are HTML files dev-test/index.html and dev-test/backends/test/index.html which contain JSON-serialized values of kitchen sink collection sample files. Am I supposed to add to those by hand?

@martinjagodic do you know who can help me answer some of these questions?

Next steps

Stuff I don't really want to do as part of this PR

  • Part 2 of the recommendation I made in feat: dependent fields #3891 (comment) - right now, you can't have variable types at the top level of an entry. I started looking into this but it's fiddly.
  • Some UI tweaks - it probably shouldn't say "0 typed object" before you've added the value
  • The refactor described above
  • A bigger refactor which gets rid of all the decap-cms-widget-* packages and just puts them in a subfolder inside decap-cms-core. I can't really see the downside of that as an end goal, but obviously it'd be a fair amount of work to actually do that, and a fair amount of work to get buy in from the maintenance team...

Checklist

Copy link

netlify bot commented Jan 12, 2024

Deploy Preview for decap-www ready!

Name Link
🔨 Latest commit bc25913
🔍 Latest deploy log https://app.netlify.com/sites/decap-www/deploys/65a16c21f713b50008a7d3a6
😎 Deploy Preview https://deploy-preview-7031--decap-www.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@martinjagodic
Copy link
Member

@mmkal this looks great. @demshy could you look into this and answer the questions?

To me, the decap-cms-widget-object-and-list and decap-cms-widget-struct are a bit confusing. Wouldn't it be simpler to have one package for both list and object instead of three?

Joining widget packages: we agree with this! But it would be a big effort for little effect (user-wise), so it wasn't very close on our radar. We would have to choose which to do first: remove immutable.js or join packages. We can't do both at the same time.

@mmkal
Copy link
Contributor Author

mmkal commented Jan 17, 2024

Good to hear - if you're open to joining packages, maybe it could start with just the list and object widgets moving to decap-cms-core for now. IMO it'd make sense to do that before moving off immutablejs - it's easier to do and centralizing generally would probably make a big dependency change lower lift.

@martinjagodic martinjagodic requested a review from a team as a code owner August 29, 2024 09:39
@martinjagodic
Copy link
Member

@mmkal I merged main and ran the tests, and it seems that all required object subfields are not required anymore - the entry is saved even when they are empty. This is apparent in the field_validations_spec test. Do you have time to look into it?

Testing: I tested it manually, but noticed there's are HTML files dev-test/index.html and dev-test/backends/test/index.html which contain JSON-serialized values of kitchen sink collection sample files. Am I supposed to add to those by hand?

I never found anything pointing to this data being generated, so I suggest that we add it manually in those index files.

To keep this moving I would address repackaging in a different PR.

@martinjagodic martinjagodic removed the request for review from a team August 29, 2024 11:51
@mmkal
Copy link
Contributor Author

mmkal commented Aug 29, 2024

I'll try to take a look in the next week. I am a bit fuzzy on the details now!

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