-
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
Changes from 2 commits
f00ba9c
4563d33
82a5c87
5278c0c
c77bd3e
1e4f0e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -301,6 +301,10 @@ const elementSpacer = css({ | |||||||||||
flex: 'none', | ||||||||||||
}); | ||||||||||||
|
||||||||||||
const readOnlySpacer = css({ | ||||||||||||
width: spacing[900], | ||||||||||||
}); | ||||||||||||
|
||||||||||||
const elementExpand = css({ | ||||||||||||
width: spacing[3], | ||||||||||||
flex: 'none', | ||||||||||||
|
@@ -371,13 +375,15 @@ export const calculateShowMoreToggleOffset = ({ | |||||||||||
editable, | ||||||||||||
level, | ||||||||||||
alignWithNestedExpandIcon, | ||||||||||||
extraGutterWidth = 0, | ||||||||||||
}: { | ||||||||||||
editable: boolean; | ||||||||||||
level: number; | ||||||||||||
alignWithNestedExpandIcon: boolean; | ||||||||||||
extraGutterWidth: number | undefined; | ||||||||||||
}) => { | ||||||||||||
// 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; | ||||||||||||
const OFFSET_WHEN_EDITABLE = editable | ||||||||||||
? // space taken by element actions | ||||||||||||
spacing[300] + | ||||||||||||
|
@@ -402,13 +408,15 @@ export const HadronElement: React.FunctionComponent<{ | |||||||||||
onEditStart?: (id: string, field: 'key' | 'value' | 'type') => void; | ||||||||||||
lineNumberSize: number; | ||||||||||||
onAddElement(el: HadronElementType): void; | ||||||||||||
extraGutterWidth?: number; | ||||||||||||
}> = ({ | ||||||||||||
value: element, | ||||||||||||
editable, | ||||||||||||
editingEnabled, | ||||||||||||
onEditStart, | ||||||||||||
lineNumberSize, | ||||||||||||
onAddElement, | ||||||||||||
extraGutterWidth, | ||||||||||||
}) => { | ||||||||||||
const darkMode = useDarkMode(); | ||||||||||||
const autoFocus = useAutoFocusContext(); | ||||||||||||
|
@@ -457,8 +465,9 @@ export const HadronElement: React.FunctionComponent<{ | |||||||||||
editable, | ||||||||||||
level, | ||||||||||||
alignWithNestedExpandIcon: true, | ||||||||||||
extraGutterWidth, | ||||||||||||
}), | ||||||||||||
[editable, level] | ||||||||||||
[editable, level, extraGutterWidth] | ||||||||||||
); | ||||||||||||
|
||||||||||||
const isValid = key.valid && value.valid; | ||||||||||||
|
@@ -567,6 +576,9 @@ export const HadronElement: React.FunctionComponent<{ | |||||||||||
</div> | ||||||||||||
</div> | ||||||||||||
)} | ||||||||||||
{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 commentThe 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
(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 commentThe 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 🤞 |
||||||||||||
{/* spacer for nested documents */} | ||||||||||||
</div> | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import type Document from 'hadron-document'; | |
import type { TypeCastMap } from 'hadron-type-checker'; | ||
import { withPreferences } from 'compass-preferences-model/provider'; | ||
import { getInsightsForDocument } from '../utils'; | ||
import { DocumentEvents } from 'hadron-document'; | ||
type BSONObject = TypeCastMap['Object']; | ||
|
||
export const documentStyles = css({ | ||
|
@@ -30,10 +31,73 @@ export type ReadonlyDocumentProps = { | |
showInsights?: boolean; | ||
}; | ||
|
||
type ReadonlyDocumentState = { | ||
expanded: boolean; | ||
}; | ||
|
||
/** | ||
* Component for a single readonly document in a list of documents. | ||
*/ | ||
class ReadonlyDocument extends React.Component<ReadonlyDocumentProps> { | ||
class ReadonlyDocument extends React.Component< | ||
ReadonlyDocumentProps, | ||
ReadonlyDocumentState | ||
> { | ||
constructor(props: ReadonlyDocumentProps) { | ||
super(props); | ||
this.state = { | ||
expanded: props.doc.expanded, | ||
}; | ||
} | ||
|
||
/** | ||
* Subscribe to the update store on mount. | ||
*/ | ||
componentDidMount() { | ||
this.subscribeToDocumentEvents(this.props.doc); | ||
} | ||
|
||
/** | ||
* Refreshing the list updates the doc in the props so we should update the | ||
* document on the instance. | ||
*/ | ||
componentDidUpdate(prevProps: ReadonlyDocumentProps) { | ||
if (prevProps.doc !== this.props.doc) { | ||
this.unsubscribeFromDocumentEvents(prevProps.doc); | ||
this.subscribeToDocumentEvents(this.props.doc); | ||
} | ||
} | ||
|
||
/** | ||
* Unsubscribe from the update store on unmount. | ||
*/ | ||
componentWillUnmount() { | ||
this.unsubscribeFromDocumentEvents(this.props.doc); | ||
} | ||
|
||
/** | ||
* Subscribe to the document events. | ||
*/ | ||
subscribeToDocumentEvents(doc: Document) { | ||
doc.on(DocumentEvents.Expanded, this.handleExpanded); | ||
doc.on(DocumentEvents.Collapsed, this.handleCollapsed); | ||
} | ||
|
||
/** | ||
* Unsubscribe from the document events. | ||
*/ | ||
unsubscribeFromDocumentEvents(doc: Document) { | ||
doc.on(DocumentEvents.Expanded, this.handleExpanded); | ||
doc.on(DocumentEvents.Collapsed, this.handleCollapsed); | ||
} | ||
|
||
handleExpanded = () => { | ||
this.setState({ expanded: true }); | ||
}; | ||
|
||
handleCollapsed = () => { | ||
this.setState({ expanded: false }); | ||
}; | ||
|
||
handleClone = () => { | ||
const clonedDoc = this.props.doc.generateObject({ | ||
excludeInternalFields: true, | ||
|
@@ -48,13 +112,32 @@ class ReadonlyDocument extends React.Component<ReadonlyDocumentProps> { | |
this.props.copyToClipboard?.(this.props.doc); | ||
}; | ||
|
||
/** | ||
* Handle clicking the expand all button. | ||
*/ | ||
handleExpandAll = () => { | ||
const { doc } = this.props; | ||
// Update the doc directly - the components internal state will update via events | ||
if (doc.expanded) { | ||
doc.collapse(); | ||
} else { | ||
doc.expand(); | ||
} | ||
}; | ||
|
||
/** | ||
* Get the elements for the document. | ||
* | ||
* @returns {Array} The elements. | ||
*/ | ||
renderElements() { | ||
return <DocumentList.Document value={this.props.doc} />; | ||
return ( | ||
<DocumentList.Document | ||
value={this.props.doc} | ||
// Provide extra whitespace for the expand button | ||
extraGutterWidth={spacing[900]} | ||
Comment on lines
+137
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
/> | ||
); | ||
} | ||
|
||
renderActions() { | ||
|
@@ -64,6 +147,8 @@ class ReadonlyDocument extends React.Component<ReadonlyDocumentProps> { | |
onClone={ | ||
this.props.openInsertDocumentDialog ? this.handleClone : undefined | ||
} | ||
onExpand={this.handleExpandAll} | ||
expanded={this.state.expanded} | ||
insights={ | ||
this.props.showInsights | ||
? getInsightsForDocument(this.props.doc) | ||
|
@@ -94,7 +179,6 @@ class ReadonlyDocument extends React.Component<ReadonlyDocumentProps> { | |
static propTypes = { | ||
copyToClipboard: PropTypes.func, | ||
doc: PropTypes.object.isRequired, | ||
expandAll: PropTypes.bool, | ||
openInsertDocumentDialog: PropTypes.func, | ||
showInsights: PropTypes.bool, | ||
}; | ||
|
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.
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):
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.