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

#7558: Improve document/form builder responsiveness #7976

Merged
merged 8 commits into from
Mar 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
103 changes: 33 additions & 70 deletions src/__snapshots__/Storyshots.test.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 0 additions & 29 deletions src/components/documentBuilder/edit/DocumentOptions.module.scss

This file was deleted.

4 changes: 2 additions & 2 deletions src/components/documentBuilder/edit/DocumentOptions.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ describe("DocumentOptions", () => {
});
});

describe("remove element", () => {
describe("remove current element", () => {
test("removes integration dependency", async () => {
// Integration dependencies included in the form state
const integrationDependencies: IntegrationDependency[] = [
Expand Down Expand Up @@ -189,7 +189,7 @@ describe("DocumentOptions", () => {

const { getFormState } = renderDocumentOptions(formState, "0");

await userEvent.click(screen.getByText("Remove element"));
await userEvent.click(screen.getByText("Remove current element"));

expect(getFormState().integrationDependencies).toStrictEqual([]);
});
Expand Down
21 changes: 8 additions & 13 deletions src/components/documentBuilder/edit/DocumentOptions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,11 @@ import useAsyncEffect from "use-async-effect";
import { useSelector } from "react-redux";
import { selectNodePreviewActiveElement } from "@/pageEditor/slices/editorSelectors";
import ElementEditor from "@/components/documentBuilder/edit/ElementEditor";
import { Col, Row } from "react-bootstrap";
import styles from "@/components/documentBuilder/edit/DocumentOptions.module.scss";
import ConnectedCollapsibleFieldSection from "@/pageEditor/fields/ConnectedCollapsibleFieldSection";
import SchemaField from "@/components/fields/schemaFields/SchemaField";
import { DOCUMENT_SCHEMA } from "@/bricks/renderers/document";
import { type Schema } from "@/types/schemaTypes";
import { UncollapsibleFieldSection } from "@/pageEditor/fields/CollapsibleFieldSection";

const DocumentOptions: React.FC<{
name: string;
Expand All @@ -55,18 +54,14 @@ const DocumentOptions: React.FC<{
{activeElement ? (
<ElementEditor documentBodyName={documentBodyName} />
) : (
<Row className={styles.currentFieldRow}>
<Col xl="3" className={styles.currentField}>
<h6>Nothing selected</h6>
</Col>
<Col xl>
<small className="text-muted">
Use the Preview Tab on the right to select an element to edit ⟶
</small>
</Col>
</Row>
<UncollapsibleFieldSection title="Document Elements">
<p className="small text-muted">
Nothing selected. Use the Preview Tab on the right to select an
element to edit ⟶
</p>
</UncollapsibleFieldSection>
)}
<ConnectedCollapsibleFieldSection title={"Advanced: Theme"}>
<ConnectedCollapsibleFieldSection title="Advanced: Theme">
<SchemaField
name={joinName(documentConfigName, "stylesheets")}
schema={DOCUMENT_SCHEMA.properties.stylesheets as Schema}
Expand Down
37 changes: 16 additions & 21 deletions src/components/documentBuilder/edit/ElementEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,9 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import styles from "./DocumentOptions.module.scss";

import { useField } from "formik";
import React from "react";
import { type DocumentElement } from "@/components/documentBuilder/documentBuilderTypes";
import { Col, Row } from "react-bootstrap";
import RemoveElement from "./RemoveElement";
import MoveElement from "./MoveElement";
import elementTypeLabels from "@/components/documentBuilder/elementTypeLabels";
Expand All @@ -42,29 +39,27 @@ const ElementEditor: React.FC<ElementEditorProps> = ({ documentBodyName }) => {
const [{ value: documentElement }] = useField<DocumentElement>(elementName);
const ElementOptions = useElementOptions(documentElement, elementName);

const currentElementName: string =
getProperty(elementTypeLabels, documentElement.type) ?? "Unknown";

return (
<>
<Row className={styles.currentFieldRow}>
<Col xl="3" className={styles.currentField}>
<h6>
{getProperty(elementTypeLabels, documentElement.type) ??
"Unknown element"}
</h6>
</Col>
<Col xl>
<ConnectedCollapsibleFieldSection
title="Document Elements"
initialExpanded
>
<p className="small text-muted">
Use the Preview Tab on the right to select an element to edit ⟶
</p>
<p>
<RemoveElement documentBodyName={documentBodyName} />
</Col>
<Col xl>
<small className="text-muted">
Use the Preview Tab on the right to select an element to edit ⟶
</small>
</Col>
</Row>
</p>

<h6>Current element: {currentElementName}</h6>
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: not a fan of using an h6. <p><strong>...</strong><p> might make more sense here?

In general, we shouldn't be skipping heading types. If we want it to be a heading, then we should use the appropriate level and re-style.

Very far from a priority.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think skipping heading levels is not an issue, especially since we don't really use them consistently. However since it's a section header, any heading tag is still preferable to p>strong IMO


<div>
<ElementOptions />
</div>
<MoveElement documentBodyName={documentBodyName} />
<MoveElement documentBodyName={documentBodyName} />
</ConnectedCollapsibleFieldSection>
<ConnectedCollapsibleFieldSection title="Advanced: Layout">
<CssSpacingField
name={joinName(elementName, "config", "className")}
Expand Down
2 changes: 1 addition & 1 deletion src/components/documentBuilder/edit/RemoveElement.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const RemoveElement: React.FC<RemoveElementProps> = ({ documentBodyName }) => {

return (
<Button onClick={onDelete} variant="danger" size="sm">
<FontAwesomeIcon icon={faTrash} /> Remove element
<FontAwesomeIcon icon={faTrash} /> Remove current element
</Button>
);
};
Expand Down
4 changes: 3 additions & 1 deletion src/components/formBuilder/FormBuilder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
import styles from "./FormBuilder.module.scss";

import React, { useState } from "react";
import FormEditor from "./edit/FormEditor";
import FormEditor from "@/components/formBuilder/edit/FormEditor";
import FormIntroFields from "@/components/formBuilder/edit/FormIntroFields";
import FormPreview from "./preview/FormPreview";
import { useField } from "formik";
import { type RJSFSchema } from "@/components/formBuilder/formBuilderTypes";
Expand All @@ -34,6 +35,7 @@ const FormBuilder: React.FC<{
return (
<div className={styles.root}>
<div className={styles.column} data-testid="editor">
<FormIntroFields name={name} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These fields are only used once or twice, I extracted this component from FormEditor

<FormEditor
name={name}
activeField={activeField}
Expand Down
86 changes: 86 additions & 0 deletions src/components/formBuilder/edit/ActiveField.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import React from "react";

Check failure on line 1 in src/components/formBuilder/edit/ActiveField.tsx

View workflow job for this annotation

GitHub Actions / strictNullChecks

strictNullChecks

src/components/formBuilder/edit/ActiveField.tsx was not found in tsconfig.strictNullChecks.json
import {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two components can't be made strict, see automated comment:
#7976 (comment)

I think I made no changes to ActiveField other than extracting it.

type SelectStringOption,
type SetActiveField,
} from "@/components/formBuilder/formBuilderTypes";
import FieldEditor from "./FieldEditor";
import {
moveStringInArray,
getNormalizedUiOrder,
} from "@/components/formBuilder/formBuilderHelpers";
import { type Schema } from "@/types/schemaTypes";
import FieldTemplate from "@/components/form/FieldTemplate";
import LayoutWidget from "@/components/LayoutWidget";
import { findLast } from "lodash";

export const ActiveField: React.FC<{
name: string;
activeField: string;
setActiveField: SetActiveField;
fieldTypes: SelectStringOption[];
schema: Schema;
uiOrder: string[];
propertyKeys: string[];
setUiOrder: (uiOrder: string[]) => Promise<void | string[]>;
}> = ({
name,
activeField,
setActiveField,
fieldTypes,
schema,
uiOrder,
propertyKeys,
setUiOrder,
}) => {
const moveProperty = async (direction: "up" | "down") => {
const nextUiOrder = moveStringInArray(
getNormalizedUiOrder(propertyKeys, uiOrder),
activeField,
direction,
);
await setUiOrder(nextUiOrder);
};

// The uiOrder field may not be initialized yet
const order = uiOrder ?? ["*"];
const canMoveUp =
order.length > 2
? order[0] !== activeField
: propertyKeys[0] !== activeField;
const canMoveDown =
order.length === propertyKeys.length + 1
? order.at(-2) !== activeField
: Array.isArray(order) &&
findLast(propertyKeys, (key) => !order.includes(key)) !== activeField;

return (
<>
<h6>Current Field</h6>

{Boolean(schema?.properties?.[activeField]) && (
<FieldEditor
name={name}
propertyName={activeField}
setActiveField={setActiveField}
fieldTypes={fieldTypes}
/>
)}

{(canMoveUp || canMoveDown) && (
<FieldTemplate
name="layoutButtons"
label="Field Order"
as={LayoutWidget}
canMoveUp={canMoveUp}
moveUp={async () => {
await moveProperty("up");
}}
canMoveDown={canMoveDown}
moveDown={async () => {
await moveProperty("down");
}}
/>
)}
</>
);
};