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-crud): allows nested elements to also expand in steps #6009

Merged
merged 14 commits into from
Jul 23, 2024

Conversation

himanshusinghs
Copy link
Contributor

@himanshusinghs himanshusinghs commented Jul 10, 2024

Description

This patch introduces "Show more" fields toggle also on the nested elements which we earlier used to render with all of their children when expanded. With the change, we will the take a similar approach as that of Document, showing only a default number of elements to begin with(set to 25) which user can later change by clicking the toggle.

Screen.Recording.2024-07-12.at.09.35.40.mov

Designs for the toggle have also changed a bit as per Slack discussion:

Before After
image image
image image

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)

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Have we talked with design about the button styles? The current one when added in the middle of the document looks very bulky compared to surrounding content, font size and family is not matching, feels out of place

@himanshusinghs himanshusinghs force-pushed the feat/nested-elements-step-expansion branch from 9fa0542 to ea98771 Compare July 12, 2024 08:50
// In the editing mode we allow to show / hide less fields because
// historically Compass was doing this for "performance" reasons
step={editing ? 100 : 1000}
onSizeChange={setVisibleFieldsCount}
></DocumentFieldsToggleGroup>
onSizeChange={handleVisibleFieldsChanged}
Copy link
Member

Choose a reason for hiding this comment

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

This is existing functionality, but I figured I'd bring it up here, no action needed. A small UX improvement here for folks with lots of fields in their documents could be to adjust the scroll position when they click to hide a lot of fields to the relative scroll height after the fields are hidden. Currently it's easy to lose a document if it's not the last document and you hide a bunch of fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I noticed this after I implemented virtual list. I will see what I can do there to modify this behaviour a little.

packages/hadron-document/src/element.ts Outdated Show resolved Hide resolved
data-testid="hide-fields-button"
>
Hide {hideSizeDiff} fields
</Button>
<Icon size="xsmall" glyph="ArrowUp"></Icon>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in the designs that Ben shared it was CaretDown / CaretUp, not an arrow. Don't have a strong preference, but arrow seems a big big when compared to the text near it, so maybe worth trying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out - I missed pulling that in, just did that.

@himanshusinghs himanshusinghs merged commit ed3980d into main Jul 23, 2024
30 checks passed
@himanshusinghs himanshusinghs deleted the feat/nested-elements-step-expansion branch July 23, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants