Skip to content

Conversation

@paula-stacho
Copy link
Collaborator

@paula-stacho paula-stacho commented Nov 21, 2025

External Links

🎟️

Description

Changes:

  • in addition to the existing onNodeExpandToggle, the client can provide onFieldExpandToggle
  • within the NodeField, the client can provide expanded
  • whether the fields expandable is calculated internally, but the toggle is only displayed if onFieldExpandToggle is provided
  • the top level toggle now has two visuals
  • the top level toggle is not controlled by the client, instead it reacts to the field state and allows expanding if any fields are collapsed
  • [BREAKING CHANGE]: for field-to-field edges, the client is to provide the source|targetFieldId instead of source|targetFieldIndex. the latter isn't suitable anymore because not all fields are visible

Note: Expandability depends on the existence of children more than on the direct types (for example there can be an object without children, or an array/mixed field that does have children).

TODO

  • make sure the height calculation uses the filtered fields

📸 Screenshots/Screencasts

Node with collapse/expand functionality

Screenshot 2025-11-21 at 16 43 34

Node without collapse/expand functionality

Screenshot 2025-11-21 at 16 43 23

Top level and field level collapse behaviour

Screen.Recording.2025-11-20.at.13.52.36.mov

@paula-stacho paula-stacho marked this pull request as ready for review November 21, 2025 15:45
disabled?: boolean;
fields: NodeField[];
fields: (NodeField & { expandable: boolean })[];
allFields: NodeField[];
Copy link
Contributor

Choose a reason for hiding this comment

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

That's an unexpected change, I'd expect NodeData to always just hold all fields, the source state, and visible fields to be derived in place where needed, otherwise you're keeping both derived and source state in one place with no clear divide between the two.

It doesn't mean that it can't be calculated once somewhere and passed down to required places, but I would suggest to not mix it into this internal type directly to make it clear that this is derived.

If you think it's absolutely unavoidable to have derived state mixed with the source state, we should at least make sure that naming is clear and fields should be renamed to something like visibleFieldsOnly with a comment clarifying that this is derived from allFields

title: string;
disabled?: boolean;
fields: NodeField[];
fields: (NodeField & { expandable: boolean })[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if "external" and "internal" types start to diverge, it would probably be better to have a dedicated type for it

Suggested change
fields: (NodeField & { expandable: boolean })[];
type InternalNodeField = NodeField & { expandable: boolean }
// ...
fields: InternalNodeField[];

Although similar to my other point, this feels like derived state, so feels off to see it here as part of the source field data type

// be filtered out as a child until we run into another element with the same
// depth (a sibling)
// We also annotate each field with whether it is expandable (has children)
// This is more reliable than checking the type of the field, since the object could be hidden in arrays, or simply have no children
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is conflating two distinct states that would be cleaner to keep separate: an object field can have no children, yes, and maybe there's some reason to do something special for the expanding functionality in that case, but this doesn't mean that the field is not expandable, just feels like a weird assumption to make. Once again, this feels like something that is better suited for a derived state in places where it's relevant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well we could make object fields expandable always, it's the other way around that's a bigger problem (it does not HAVE to be an object for it to be expandable).. But I also thought it'd be confusing to have the icon there doing nothing (in the case of object with no children). I could rename it to hasChildren?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's like both ways, just seems like two separate concepts: something can be theoretically expandable (object, array with object type) and there's actually some state that would make sense to allow expand collapse. I think name change to hasChildren makes it better, yes, makes it very clear what we're caculating here

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.

3 participants