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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Nov 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f00ba9c
Make ReadonlyDocument expandable
kraenhansen Sep 19, 2024
4563d33
Add extra gutter width on ReadonlyDocument
kraenhansen Nov 5, 2024
82a5c87
fixup! Add extra gutter width on ReadonlyDocument
kraenhansen Nov 5, 2024
5278c0c
fixup! Add extra gutter width on ReadonlyDocument
kraenhansen Nov 5, 2024
c77bd3e
fixup! Add extra gutter width on ReadonlyDocument
kraenhansen Nov 6, 2024
1e4f0e2
fixup! Add extra gutter width on ReadonlyDocument
kraenhansen Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/compass-components/src/components/document-list/document.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,14 @@ const HadronDocument: React.FunctionComponent<{
editable?: boolean;
editing?: boolean;
onEditStart?: () => void;
}> = ({ value: document, editable = false, editing = false, onEditStart }) => {
extraGutterWidth?: number;
}> = ({
value: document,
editable = false,
editing = false,
onEditStart,
extraGutterWidth,
}) => {
const { elements, visibleElements } = useHadronDocument(document);
const [autoFocus, setAutoFocus] = useState<{
id: string;
Expand Down Expand Up @@ -147,6 +154,7 @@ const HadronDocument: React.FunctionComponent<{
type: el.parent?.currentType === 'Array' ? 'value' : 'key',
});
}}
extraGutterWidth={extraGutterWidth}
></HadronElement>
);
})}
Expand Down
17 changes: 15 additions & 2 deletions packages/compass-components/src/components/document-list/element.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,10 @@ const elementSpacer = css({
flex: 'none',
});

const readOnlySpacer = css({
width: spacing[900],
});

const elementExpand = css({
width: spacing[3],
flex: 'none',
Expand Down Expand Up @@ -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;
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.

const OFFSET_WHEN_EDITABLE = editable
? // space taken by element actions
spacing[300] +
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -567,6 +576,9 @@ export const HadronElement: React.FunctionComponent<{
</div>
</div>
)}
{typeof extraGutterWidth === 'number' && (
<div style={{ width: extraGutterWidth }} />
)}
<div className={elementSpacer} style={{ width: elementSpacerWidth }}>
Copy link
Collaborator

@gribnoysup gribnoysup Nov 6, 2024 •
edited
Loading

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 🤞

{/* spacer for nested documents */}
</div>
Expand Down Expand Up @@ -711,6 +723,7 @@ export const HadronElement: React.FunctionComponent<{
onEditStart={onEditStart}
lineNumberSize={lineNumberSize}
onAddElement={onAddElement}
extraGutterWidth={extraGutterWidth}
></HadronElement>
);
})}
Expand Down
90 changes: 87 additions & 3 deletions packages/compass-crud/src/components/readonly-document.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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,
Expand All @@ -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
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.

/>
);
}

renderActions() {
Expand All @@ -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)
Expand Down Expand Up @@ -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,
};
Expand Down
Loading