-
Notifications
You must be signed in to change notification settings - Fork 180
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
Conversation
}) => { | ||
// 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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
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 🤔
// Provide extra whitespace for the expand button | ||
extraGutterWidth={spacing[900]} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Pass extraGutterWidth to calculateShowMoreToggleOffset in document
My bad again. It seems I haven't got sufficient test data to trigger these edge-cases. I forgot to pass the
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
{typeof extraGutterWidth === 'number' && ( | ||
<div style={{ width: extraGutterWidth }} /> | ||
)} | ||
<div className={elementSpacer} style={{ width: elementSpacerWidth }}> |
There was a problem hiding this comment.
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:
{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
There was a problem hiding this comment.
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 🤞
Use existing element spacer
? // 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) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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:
ReadonlyDocument
expandable, by registering relevant event listeners on thedoc
updating a local state.DocumentActionsGroup
component to render the "expand button".Element
component when used in aReadonlyDocument
to avoid conflicts with the field-level expand/collapse icons.Screen.Recording.2024-11-05.at.12.54.59.mov
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes