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

feat: Updated GraphQL form UI #36728

Merged
merged 7 commits into from
Oct 10, 2024
Merged

feat: Updated GraphQL form UI #36728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a88c13
feat: Updated graphQL to use dynamic text feild instead of custom UI
albinAppsmith Oct 7, 2024
c2496b5
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Oct 7, 2024
a44dd03
fix: fixed missing classname for cypress test
albinAppsmith Oct 8, 2024
c5dbe1b
fix: Removed unnecessary files
albinAppsmith Oct 8, 2024
602a932
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Oct 8, 2024
ddaf538
fix: Removed dependency of tab heights from api editor
albinAppsmith Oct 8, 2024
14b3e00
Merge branch 'release' of https://github.com/appsmithorg/appsmith int…
albinAppsmith Oct 8, 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
148 changes: 64 additions & 84 deletions .../PluginActionEditor/components/PluginActionForm/components/GraphQLEditor/PostBodyData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,107 +1,87 @@
import React, { useCallback, useRef } from "react";
import React from "react";
import styled from "styled-components";
import QueryEditor from "pages/Editor/APIEditor/GraphQL/QueryEditor";
import VariableEditor from "pages/Editor/APIEditor/GraphQL/VariableEditor";
import useHorizontalResize from "utils/hooks/useHorizontalResize";
import { EditorTheme } from "components/editorComponents/CodeEditor/EditorConfig";
import classNames from "classnames";
import { tailwindLayers } from "constants/Layers";

const ResizableDiv = styled.div`
display: flex;
height: 100%;
flex-shrink: 0;
`;
import {
CodeEditorBorder,
EditorModes,
EditorSize,
EditorTheme,
TabBehaviour,
} from "components/editorComponents/CodeEditor/EditorConfig";
import DynamicTextField from "components/editorComponents/form/fields/DynamicTextField";
import { Section, Zone } from "pages/Editor/ActionForm";
import { AutocompleteDataType } from "utils/autocomplete/AutocompleteDataType";
import FormLabel from "components/editorComponents/FormLabel";

const PostBodyContainer = styled.div`
display: flex;
height: 100%;
overflow: hidden;
&&&& .CodeMirror {
height: 100%;
border-top: 1px solid var(--ads-v2-color-border);
border-bottom: 1px solid var(--ads-v2-color-border);
border-radius: 0;
padding: 0;
}
& .CodeMirror-scroll {
margin: 0px;
padding: 0px;
overflow: auto !important;
height: auto;
min-height: 250px;
}
`;

const ResizerHandler = styled.div<{ resizing: boolean }>`
width: 6px;
height: 100%;
margin-left: 2px;
border-right: 1px solid var(--ads-v2-color-border);
background: ${(props) =>
props.resizing ? "var(--ads-v2-color-border)" : "transparent"};
&:hover {
background: var(--ads-v2-color-border);
border-color: transparent;
const StyledFormLabel = styled(FormLabel)`
&& {
margin-bottom: var(--ads-v2-spaces-2);
padding: 0;
}
`;

const DEFAULT_GRAPHQL_VARIABLE_WIDTH = 300;

interface Props {
actionName: string;
}

const EXPECTED_VARIABLE = {
type: "object",
example:
'{\n "name":"{{ inputName.property }}",\n "preference":"{{ dropdownName.property }}"\n}',
autocompleteDataType: AutocompleteDataType.OBJECT,
};

function PostBodyData(props: Props) {
const { actionName } = props;
const theme = EditorTheme.LIGHT;
const resizeableRef = useRef<HTMLDivElement>(null);
const [variableEditorWidth, setVariableEditorWidth] = React.useState(
DEFAULT_GRAPHQL_VARIABLE_WIDTH,
);
/**
* Variable Editor's resizeable handler for the changing of width
*/
const onVariableEditorWidthChange = useCallback((newWidth) => {
setVariableEditorWidth(newWidth);
}, []);

const { onMouseDown, onMouseUp, onTouchStart, resizing } =
useHorizontalResize(
resizeableRef,
onVariableEditorWidthChange,
undefined,
true,
);

return (
<PostBodyContainer>
<QueryEditor
dataTreePath={`${actionName}.config.body`}
height="100%"
name="actionConfiguration.body"
theme={theme}
/>
<div
className={`w-2 h-full -ml-2 group cursor-ew-resize ${tailwindLayers.resizer}`}
onMouseDown={onMouseDown}
onTouchEnd={onMouseUp}
onTouchStart={onTouchStart}
>
<ResizerHandler
className={classNames({
"transform transition": true,
})}
resizing={resizing}
/>
</div>
<ResizableDiv
ref={resizeableRef}
style={{
width: `${variableEditorWidth}px`,
paddingRight: "2px",
}}
>
<VariableEditor actionName={actionName} theme={theme} />
</ResizableDiv>
<Section isFullWidth>
<Zone layout="single_column">
<div className="t--graphql-query-editor">
<StyledFormLabel>Query</StyledFormLabel>
<DynamicTextField
border={CodeEditorBorder.ALL_SIDE}
dataTreePath={`${actionName}.config.body`}
evaluatedPopUpLabel={"Query"}
mode={EditorModes.GRAPHQL_WITH_BINDING}
name="actionConfiguration.body"
placeholder={`{{\n\t{name: inputName.property, preference: dropdownName.property}\n}}`}
showLineNumbers
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Provide an Appropriate Placeholder for the Query Input

Dear students, when guiding users through placeholders, it's essential to offer examples that closely resemble the expected input. The current placeholder for the Query input appears to be an object, which might confuse users expecting to enter a GraphQL query. Let's refine it to present a sample GraphQL query instead.

Consider updating the placeholder as follows:

- placeholder={`{{\n\t{name: inputName.property, preference: dropdownName.property}\n}}`}
+ placeholder={`query {\n  user(id: "{{inputName.property}}") {\n    name\n    email\n  }\n}`}

This change provides a clearer example of a GraphQL query, enhancing user understanding.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
placeholder={`{{\n\t{name: inputName.property, preference: dropdownName.property}\n}}`}
placeholder={`query {\n user(id: "{{inputName.property}}") {\n name\n email\n }\n}`}

size={EditorSize.EXTENDED}
tabBehaviour={TabBehaviour.INDENT}
theme={theme}
/>
</div>
</Zone>
<Zone layout="single_column">
<div className="t--graphql-variable-editor">
<StyledFormLabel>Query variables</StyledFormLabel>
<DynamicTextField
border={CodeEditorBorder.ALL_SIDE}
dataTreePath={`${props.actionName}.config.pluginSpecifiedTemplates[1].value`}
evaluatedPopUpLabel={"Query variables"}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Maintain Consistency in Accessing actionName

Class, consistency is key to writing clean and maintainable code. Throughout your component, you've destructured actionName from props, yet in line 69, you revert to using props.actionName. Let's align this usage for clarity.

Apply the following change:

- dataTreePath={`${props.actionName}.config.pluginSpecifiedTemplates[1].value`}
+ dataTreePath={`${actionName}.config.pluginSpecifiedTemplates[1].value`}

This adjustment ensures uniformity in how actionName is referenced.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
dataTreePath={`${props.actionName}.config.pluginSpecifiedTemplates[1].value`}
dataTreePath={`${actionName}.config.pluginSpecifiedTemplates[1].value`}

expected={EXPECTED_VARIABLE}
height="100%"
mode={EditorModes.JSON_WITH_BINDING}
name="actionConfiguration.pluginSpecifiedTemplates[1].value"
placeholder={`${EXPECTED_VARIABLE.example}\n\n\\\\Take widget inputs using {{ }}`}
showLightningMenu={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the Escape Characters in the Placeholder String

Attention class, proper string formatting is crucial for accurate rendering. In line 75, the placeholder string includes double backslashes (\\\\), which may not display as intended in the UI. Let's correct this to ensure the placeholder message is clear to the users.

Here's the suggested fix:

- placeholder={`${EXPECTED_VARIABLE.example}\n\n\\\\Take widget inputs using {{ }}`}
+ placeholder={`${EXPECTED_VARIABLE.example}\n\n\\Take widget inputs using {{ }}`}

This change adjusts the escape characters so that a single backslash is rendered, providing the correct guidance to users.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
placeholder={`${EXPECTED_VARIABLE.example}\n\n\\\\Take widget inputs using {{ }}`}
placeholder={`${EXPECTED_VARIABLE.example}\n\n\\Take widget inputs using {{ }}`}

showLineNumbers
size={EditorSize.EXTENDED}
tabBehaviour={TabBehaviour.INDENT}
theme={theme}
/>
</div>
</Zone>
</Section>
</PostBodyContainer>
);
}
Expand Down
1 change: 1 addition & 0 deletions app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ const Wrapper = styled.div`
flex-direction: row;
height: 100%;
position: relative;
overflow: hidden;
`;

const MainContainer = styled.div`
Expand Down
3 changes: 2 additions & 1 deletion app/client/src/pages/Editor/APIEditor/Editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,10 @@ class ApiEditor extends React.Component<Props> {

const formStyles: CSSProperties = {
position: "relative",
height: "100%",
display: "flex",
flexDirection: "column",
flexGrow: "1",
overflow: "auto",
};

// TODO: Fix this the next time the file is edited
Expand Down
67 changes: 0 additions & 67 deletions app/client/src/pages/Editor/APIEditor/GraphQL/QueryEditor.tsx
View file
Open in desktop

This file was deleted.

77 changes: 0 additions & 77 deletions app/client/src/pages/Editor/APIEditor/GraphQL/VariableEditor.tsx
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions app/client/src/pages/Editor/ActionForm/Section/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import styles from "./styles.module.css";
interface SectionProps extends React.HTMLAttributes<HTMLDivElement> {
children: React.ReactNode;
isStandalone?: boolean;
isFullWidth?: boolean;
}

const Section: React.FC<SectionProps> = ({
children,
className,
isFullWidth = false,
isStandalone = false,
...props
}) => {
Expand All @@ -18,6 +20,7 @@ const Section: React.FC<SectionProps> = ({
return (
<div
className={classNames}
data-fullwidth={isFullWidth.toString()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not using classes to control css states?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point... This was a practise we used in ADS as well I see in WDS. @KelvinOm Do you see any specific reason we should stick to this? As I can see, class is better performant than data attributes.

data-standalone={isStandalone.toString()}
{...props}
>
Expand Down
4 changes: 4 additions & 0 deletions app/client/src/pages/Editor/ActionForm/Section/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@
&[data-standalone="false"]:not(:last-child) {
border-bottom: 1px solid var(--ads-v2-color-border);
}

&[data-fullwidth="true"] {
max-width: none;
}
}
Loading