feat: Updated GraphQL form UI #36728
Conversation
…o action-redesign/graphql-zone-section
WalkthroughThe pull request introduces significant changes to various components within the application, particularly focusing on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
github-actions
bot
added
Enhancement
github-actions
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/Editor/ActionForm/Section/styles.module.css (1)
17-19: Well done, class! This addition expands our layout possibilities.The new style rule for full-width sections is a valuable enhancement to our CSS toolkit. Let's break it down:
- We're using a data attribute
data-fullwidth="true"to control the width. This is an excellent practice as it separates our styling concerns from our HTML structure.- By setting
max-width: none;, we're allowing the section to expand beyond its default 800px limit when needed.This change aligns perfectly with our lesson objective of creating more flexible and responsive layouts. It's like giving our sections the ability to stretch their arms wide when they need more space!
Remember, class, with great power comes great responsibility. Use this full-width option judiciously to maintain a balanced and harmonious layout in your designs.
app/client/src/PluginActionEditor/components/PluginActionForm/components/GraphQLEditor/PostBodyData.tsx (1)
17-18: Consider Responsive Design for Code Editor HeightDear students, setting a minimum height for components can enhance the user experience by ensuring sufficient space is available for content. In lines 17-18, you've set
height: auto;andmin-height: 250px;for the CodeMirror editor. This is a good practice, but let's also consider how this affects smaller screens.Review the component's behavior on various screen sizes to ensure it's responsive. Adjust the
min-heightif necessary to accommodate smaller devices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/src/PluginActionEditor/components/PluginActionForm/components/GraphQLEditor/PostBodyData.tsx (1 hunks)
- app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (1 hunks)
- app/client/src/pages/Editor/APIEditor/Editor.tsx (2 hunks)
- app/client/src/pages/Editor/ActionForm/Section/index.tsx (2 hunks)
- app/client/src/pages/Editor/ActionForm/Section/styles.module.css (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/pages/Editor/APIEditor/Editor.tsx
🧰 Additional context used
🔇 Additional comments (6)
app/client/src/pages/Editor/ActionForm/Section/index.tsx (4)
8-8: Well done, class! Let's examine this new addition.The introduction of the
isFullWidthprop is a commendable step towards enhancing our component's flexibility. This optional boolean property will allow us to control the width behavior of our Section component, which is crucial for responsive design.Remember, children, in React, props are the building blocks of component customization. By adding this prop, we're giving future developers more control over the layout of their sections.
14-14: Excellent use of default values, students!Here, we see a prime example of how to handle new props responsibly. By setting
isFullWidth = falseas the default, we ensure that our component maintains its current behavior for all existing implementations. This is what we call "backwards compatibility" in the programming world.Remember, class: when adding new features, always consider how they might affect existing code. Default values are our friends in such situations!
23-23: Attention, class! Let's discuss this clever use of data attributes.The addition of
data-fullwidth={isFullWidth.toString()}is a textbook example of how to expose component state for styling or scripting purposes. By using a data attribute, we're following best practices in separating our concerns.Note the use of
.toString()here. Can anyone tell me why this is necessary? That's right! HTML attributes expect string values, so we need to convert our boolean to a string.This approach allows us to style our component differently based on its
isFullWidthstate using CSS attribute selectors. For example:.section[data-fullwidth="true"] { width: 100%; }Well done on implementing this feature so thoughtfully!
Line range hint
1-33: Class, let's summarize what we've learned today!This pull request has successfully enhanced our Section component by introducing the
isFullWidthprop. Let's recap the key points:
- We added a new optional prop to our interface, expanding the component's API.
- We set a sensible default value, ensuring backwards compatibility.
- We exposed the prop's value through a data attribute, allowing for flexible styling.
These changes directly address the objectives outlined in issue #36499, providing greater flexibility in layout design. The ability to create full-width sections will undoubtedly improve the responsiveness and visual appeal of our forms.
Remember, students, small changes like these can have a big impact on the overall user experience. Always think about how your code changes can make life easier for both developers and end-users.
Great work on this implementation! Does anyone have any questions before we move on?
app/client/src/PluginActionEditor/components/PluginActionForm/components/GraphQLEditor/PostBodyData.tsx (1)
72-72: Evaluate the Necessity of theheightPropertyStudents, it's prudent to question whether each property we add serves a purpose. The
height="100%"attribute on theDynamicTextFieldmight be redundant if the component naturally adapts its height based on content.Please verify if the
heightprop is necessary. If it's not influencing the component's display, consider removing it to simplify the code.app/client/src/pages/Editor/APIEditor/CommonEditorForm.tsx (1)
114-114: Ensureoverflow: hidden;Does Not Hide Important ContentDear student, by adding
overflow: hidden;to theWrappercomponent, you're controlling the overflow behavior of this element. While this can prevent unwanted scroll bars and hide overflowing content, please double-check to ensure this does not unintentionally conceal any important UI elements or disrupt the user experience, especially on smaller screens or when the content dynamically changes.
| height="100%" | ||
| mode={EditorModes.JSON_WITH_BINDING} | ||
| name="actionConfiguration.pluginSpecifiedTemplates[1].value" | ||
| placeholder={`${EXPECTED_VARIABLE.example}\n\n\\\\Take widget inputs using {{ }}`} |
There was a problem hiding this comment.
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.
| placeholder={`${EXPECTED_VARIABLE.example}\n\n\\\\Take widget inputs using {{ }}`} | |
| placeholder={`${EXPECTED_VARIABLE.example}\n\n\\Take widget inputs using {{ }}`} |
| evaluatedPopUpLabel={"Query"} | ||
| mode={EditorModes.GRAPHQL_WITH_BINDING} | ||
| name="actionConfiguration.body" | ||
| placeholder={`{{\n\t{name: inputName.property, preference: dropdownName.property}\n}}`} |
There was a problem hiding this comment.
🛠️ 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.
| placeholder={`{{\n\t{name: inputName.property, preference: dropdownName.property}\n}}`} | |
| placeholder={`query {\n user(id: "{{inputName.property}}") {\n name\n email\n }\n}`} |
| <StyledFormLabel>Query variables</StyledFormLabel> | ||
| <DynamicTextField | ||
| border={CodeEditorBorder.ALL_SIDE} | ||
| dataTreePath={`${props.actionName}.config.pluginSpecifiedTemplates[1].value`} |
There was a problem hiding this comment.
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.
| dataTreePath={`${props.actionName}.config.pluginSpecifiedTemplates[1].value`} | |
| dataTreePath={`${actionName}.config.pluginSpecifiedTemplates[1].value`} |
| @@ -226,7 +227,7 @@ class ApiEditor extends React.Component<Props> { | |||
|
|
|||
| const formStyles: CSSProperties = { | |||
| position: "relative", | |||
| height: "100%", | |||
| height: `calc(100% - ${EDITOR_TABS_HEIGHT})`, | |||
There was a problem hiding this comment.
Can we do this without depending on the height of the tabs? This makes it quite dependent on tabs. As you know all implementations do not have tabs
…o action-redesign/graphql-zone-section
…o action-redesign/graphql-zone-section
| @@ -18,6 +20,7 @@ const Section: React.FC<SectionProps> = ({ | |||
| return ( | |||
| <div | |||
| className={classNames} | |||
| data-fullwidth={isFullWidth.toString()} | |||
There was a problem hiding this comment.
Any reason for not using classes to control css states?
There was a problem hiding this comment.
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.
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11250281358. |
|
Deploy-Preview-URL: https://ce-36728.dp.appsmith.com |
Description
This PR has following new features and fixes
EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5297
Fixes #35494
Fixes #36410
Fixes #36499
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11240771681
Commit: 14b3e00
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 09 Oct 2024 05:42:42 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
PostBodyDatacomponent, enhancing user interaction with dynamic text fields for GraphQL queries and variables.MainContainer, to improve layout responsiveness in theCommonEditorForm.Sectioncomponent with an optionalisFullWidthproperty for greater configurability.Bug Fixes
LoadingContainerheight dynamically to enhance UI responsiveness.Style
Sectioncomponent to support full-width expansion when applicable.Removed Features
QueryEditorandVariableEditorcomponents, simplifying the editor interface.