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(compass-editor): expandable json editor #6446

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

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Nov 5, 2024

Description

A sub-task of the design proposed in COMPASS-4635 is to add the "expand all" / "collapse all" button to the JSON editor view of documents.

Merging this PR will:

  • Update the spacing of the actions container and styles injected by the JSONEditor component to use the non-deprecated variants, as well as pull the actions container to span the entire width.
  • Add pointer-events css properties to avoid capturing clicks on whitespace of the actions container.
  • Add a "compact" prop to the editors ActionButton component, using the same hack introduced in #6439. I hope to replace this by an upstream change to leafygreen-ui.
  • Add the "Expand all" / "Collapse all" button to the ActionsContainer conditionally rendered on the presence of an onExpend prop.
  • Pass a callback for the onExpend from the JSONEditor into the editor, which calls the doc's collapse / expand methods.
  • Adds 20px of spacing to the gutters of the editor when the "expand all" button is visible to prevent collisions with the fold / unfold icons in the gutter.

Notice how the "dismissal" behaviour is a bit different in the JSON editor than the regular document editor: As the focus is brought to the action button when clicking it, it will prevent the action group from disappearing. I.e. it is not enough to simply move the mouse pointer outside of the element to get the action buttons to hide. This is not new functionality but the effect of the existing styling here:

[`&:focus-within > .multiline-editor-actions,

Screen.Recording.2024-11-05.at.09.32.12.mov

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@kraenhansen kraenhansen added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Nov 5, 2024
@kraenhansen kraenhansen self-assigned this Nov 5, 2024
@github-actions github-actions bot added the feat label Nov 5, 2024
@gribnoysup
Copy link
Contributor

gribnoysup commented Nov 5, 2024

I'm a bit confused, so just double-checking: in the designs the expand / collapse button is added to the "Document" read-only view (which IIRC was the main user complaint, because JSON editor already has a top level expand button, so this one is just duplicating it more or less), but this PR adds it to the JSON view, there will be a follow-up?

EDIT: actually, if we're adding this here as an action button, should we somehow hide the button in the editor itself in these cases? Showing two almost identical in behavior buttons feels a bit confusing

Also in the JSON view in this implementation it aligns a bit weird with the document itself:

image

In the designs the button is aligne by the middle of the first line of the editor:

image

Was there a change in the design that's just not documented?

@kraenhansen
Copy link
Contributor Author

kraenhansen commented Nov 5, 2024

JSON editor already has a top level expand button

The button on the top level doesn't expand all the child fields.

should we somehow hide the button in the editor itself in these cases?

it aligns a bit weird with the document itself

Was there a change in the design that's just not documented?

I'd defer those decisions to Diancheng: I've asked for her opinion.

@gribnoysup
Copy link
Contributor

gribnoysup commented Nov 5, 2024

The button on the top level doesn't expand all the child fields.

Sorry, I didn't communicate what's confusing here well, my bad. To clarify: we have two buttons with the same icon on the same line / level in UI doing slightly different things, how do user understands the difference and do they need to? Is there a point in having two? Might be worth asking Diancheng about this too

@dianchenghu
Copy link

The existing caret button is for fold/unfold code block; and the new one we are adding here is for expand/collapse all embedded fields. Both of them have tooltips when hovering, and it's easy enough to learn after clicking.
I am still trying to understand why we need the fold/unfold feature; it was built before I joined. I'm not against the idea of removing it. @betsybutton What are your thoughts? But I think it should not be a blocker for adding the new button.
Here's the user request for what this PR is solving.
https://feedback.mongodb.com/forums/924283-compass/suggestions/46586038-expand-all-in-json-view-should-be-brought-back

Yes, we will hide the expand all button in the editor view.

For the alignment, since the JSON view needs less space than the list view, I leave 8px on the sides of the border in the design. So it might look more like this (the green one):
image

@gribnoysup
Copy link
Contributor

Here's the user request for what this PR is solving. https://feedback.mongodb.com/forums/924283-compass/suggestions/46586038-expand-all-in-json-view-should-be-brought-back

Yeah, so it seems like we broke the old behavior by switching editors and user wants the existing top level button to expand / collapse all and not just one level (instead of an extra button). It's just a handful of people who upvoted, so having two buttons is probably okay if it looks okay to you from design perspective

@kraenhansen
Copy link
Contributor Author

I've adjusted the offset of the expand/collapse button to match the design.

Screenshot 2024-11-05 at 14 36 47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants