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

fix(crud): expandable readonly document COMPASS-4635 #6447

Merged
merged 6 commits into from
Nov 8, 2024

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Nov 5, 2024

Description

This is the final sub-task of COMPASS-4635 adding an "expand all" / "collapse all" button to the read-only view.

Merging this PR will:

  • Make the ReadonlyDocument expandable, by registering relevant event listeners on the doc updating a local state.
  • Pass a callback into the DocumentActionsGroup component to render the "expand button".
  • Add extra gutter width in the Element component when used in a ReadonlyDocument to avoid conflicts with the field-level expand/collapse icons.
Screen.Recording.2024-11-05.at.12.54.59.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 self-assigned this Nov 5, 2024
@kraenhansen kraenhansen changed the title fix(compass-crud): expandable readonly document COMPASS-4635 fix(crud): expandable readonly document COMPASS-4635 Nov 5, 2024
}) => {
// the base padding that we have on all elements rendered in the document
const BASE_PADDING_LEFT = spacing[50];
const BASE_PADDING_LEFT = spacing[50] + extraGutterWidth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This either needs a default value (or maybe should be done outside of this component altogether), otherwise you're breaking the padding when value is not provided (left is this branch):

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line you're referencing is used only to calculate the offset of the "Show more" link when displaying long lists of values. But you're right something was broken: I wasn't passing the extraGutterWidth through to the child elements of an element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the video in the description.

Passing extraGutterWidth through to children
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.

Seems like in readonly the extra fields button is now misaligned

image

Still thinking that maybe if we're trying to add a uniform gutter to the whole Document here to everything, there might be a better place for that than manually passing it to all the nested elements like Element, there is probably a spot that doesn't require us to track down the value in multiple places and only do it in one 🤔

Comment on lines +137 to +138
// Provide extra whitespace for the expand button
extraGutterWidth={spacing[900]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want this to be very flexible in the component interface we probably should note that this value should match exactly the space left by hidden editing action buttons in the editable card, so that in case the editable card gets updated we can find this and update the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there a requirement that this offset match any other state as such. Just that it's enough for the expand button to not overlay the icons of the individual elements.

@nirinchev nirinchev changed the title fix(crud): expandable readonly document COMPASS-4635 fix(crud): expandable readonly document COMPASS-4635 Nov 5, 2024
@github-actions github-actions bot added the fix label Nov 5, 2024
Pass extraGutterWidth to calculateShowMoreToggleOffset in document
@kraenhansen
Copy link
Contributor Author

kraenhansen commented Nov 5, 2024

Seems like in readonly the extra fields button is now misaligned

My bad again. It seems I haven't got sufficient test data to trigger these edge-cases. I forgot to pass the extraGutterWidth to the calculateShowMoreToggleOffset call in the HadronDocument component: 5278c0c

there might be a better place for that than manually passing it to all the nested elements like Element

Perhaps. I haven't found one. The obvious would be a paddingLeft somewhere in the document, but that messes up the effect (gray background) when hovering an element in the document.

Remove unused readOnlySpacer CSS
Comment on lines 575 to 578
{typeof extraGutterWidth === 'number' && (
<div style={{ width: extraGutterWidth }} />
)}
<div className={elementSpacer} style={{ width: elementSpacerWidth }}>
Copy link
Collaborator

@gribnoysup gribnoysup Nov 6, 2024

Choose a reason for hiding this comment

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

Nit: this element is known to be a performance bottleneck due to the sheer amount of dom elements it might end up rendering on the page (yeah, even if they are just empty divs, "expand all" on a massive document and you're rendering thousands of these), so one thing I'd suggest here is to combine your extra width into the existing element:

Suggested change
{typeof extraGutterWidth === 'number' && (
<div style={{ width: extraGutterWidth }} />
)}
<div className={elementSpacer} style={{ width: elementSpacerWidth }}>
<div className={elementSpacer} style={{ width: elementSpacerWidth + (extraGutterWidth ?? 0) }}>

(or you can include it in the existing calculate extra width function above)

It's less of an issue now that we're virtualizing the list of these document cards in most cases, but doesn't hurt to be a bit more vigilant about that in this particular element.

Also if you want to keep the extra element, you might want to add the same elementSpacer class to it to make sure that it won't get the default flex shrink value that would allow it to be resized inside of this flex parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to use the existing spacer and fixed what I believe was a bug in the calculation of the spacer width 🤞

@kraenhansen
Copy link
Contributor Author

I've updated the code to use the existing element spacer 👍

Here's a screenshot of the "show more" links on fields and array items:

Screenshot 2024-11-06 at 13 35 09 Screenshot 2024-11-06 at 13 45 35

? // space taken by element actions
spacing[300] +
// space and margin taken by line number element
spacing[400] +
spacing[100] +
// element spacer width that we render
calculateElementSpacerWidth(editable, level)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this takes the editable as an argument and displaying the spacer isn't conditional on the value of editable I've moved it out of the OFFSET_WHEN_EDITABLE (now editableOffset).

@@ -378,23 +382,20 @@ export const calculateShowMoreToggleOffset = ({
alignWithNestedExpandIcon: boolean;
extraGutterWidth: number | undefined;
}) => {
// the base padding that we have on all elements rendered in the document
const BASE_PADDING_LEFT = spacing[50] + extraGutterWidth;
Copy link
Contributor Author

@kraenhansen kraenhansen Nov 6, 2024

Choose a reason for hiding this comment

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

I've updated the casing (and namig) on these local variables, since they are not global constants.

@kraenhansen kraenhansen merged commit c179bd4 into main Nov 8, 2024
30 checks passed
@kraenhansen kraenhansen deleted the kh/expandable-readonly-document branch November 8, 2024 15:31
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.

2 participants