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

fix: adjust spacing for filter/sort/pagination form controls #37221

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

alex-golovanov
Copy link
Contributor

@alex-golovanov alex-golovanov commented Nov 5, 2024

Description

Adjusted spacing to match design values for filter/sort/pagination form controls.

Fixes #37198

Automation

/ok-to-test tags="@tag.IDE"

🔍 Cypress test results

Tip

🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11699699613
Commit: cb18e1e
Cypress dashboard.
Tags: @tag.IDE
Spec:


Wed, 06 Nov 2024 08:54:32 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced layout responsiveness for the Zone, Pagination, Sorting, and Where Clause components.
    • Improved clarity and usability of pagination feature in the Google Sheets plugin.
  • Bug Fixes

    • Corrected a typo in the WhereClauseControl component function name.
  • Style

    • Updated CSS for various components to use CSS variables for consistent spacing.
    • Removed hardcoded widths for improved flexibility in form controls.
    • Added border-radius and adjusted padding for the SecondaryBox component.
    • Streamlined rendering logic for form configuration elements.
  • Refactor

    • Minor refactoring of function names and properties for better clarity and maintainability.

Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Walkthrough

The pull request introduces various modifications across multiple components, primarily focusing on styling enhancements and layout adjustments. Key changes include the removal of hardcoded widths, the introduction of CSS variables for spacing, and the enhancement of responsiveness in the Zone, PaginationControl, SortingControl, and WhereClauseControl components. These updates streamline the layout and improve the overall flexibility of the components without altering their core functionalities.

Changes

File Change Summary
.../Zone/styles.module.css Adjusted layout properties, removed hardcoded widths, and set empty child div elements to display: none.
.../PaginationControl.tsx Removed fixed widths, updated grid properties for responsiveness, and enhanced visual structure.
.../SortingControl.tsx Replaced fixed gap values with CSS variables and adjusted styles for better responsiveness.
.../WhereClauseControl.tsx Updated styling properties to use CSS variables, corrected function name typo, and improved type safety.
.../Editor/FormConfig.tsx Simplified rendering logic for form labels and adjusted tooltip rendering without altering core behavior.
.../googleSheetsPlugin/src/main/resources/editor/fetch_many.json Added a new label property for pagination and expanded tooltip text for clarity in the Google Sheets plugin.

Possibly related PRs

Suggested labels

UI Improvement, Low, Production, Needs Triaging, IDE Navigation

Suggested reviewers

  • hetunandu
  • albinAppsmith

🎉 In the realm of code, where styles do play,
We’ve trimmed the widths, let layouts sway.
With gaps now set by variables bright,
Our components dance, responsive and light!
So here’s to the changes, both bold and keen,
In the world of CSS, we reign supreme! 🌟


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a9b10b8 and cb18e1e.

📒 Files selected for processing (2)
  • app/client/src/pages/Editor/FormConfig.tsx (1 hunks)
  • app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/editor/fetch_many.json (1 hunks)
🔇 Additional comments (3)
app/server/appsmith-plugins/googleSheetsPlugin/src/main/resources/editor/fetch_many.json (2)

201-201: LGTM: Empty label addresses spacing issue.

The empty label property aligns with the requested changes to remove empty space above the pagination field.


Line range hint 201-217: Verify pagination configuration with UI components.

The pagination configuration looks good with clear tooltips and proper binding examples. Let's verify the changes work correctly with the UI components.

app/client/src/pages/Editor/FormConfig.tsx (1)

207-207: Verify label rendering behavior for edge cases.

The modified condition label?.length > 0 || encrypted || shouldRenderSubtitle is more permissive than before. While this simplifies the logic, we should ensure it doesn't cause unintended layout issues.

✅ Verification successful

Label rendering condition is correctly implemented

The modified condition is consistent with the component's design and usage across the codebase. The shouldRenderSubtitle logic is properly integrated with form label rendering, and the condition handles all necessary cases including encrypted fields and subtitle display. No unintended layout issues are expected.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other components that might be affected by this label rendering change
rg -l "FormLabel|nestedFormControl" --type tsx

# Look for potential test cases covering this scenario
rg -l "nested.*form.*control.*test" --type tsx

Length of output: 153


Script:

#!/bin/bash
# Search for other components that might be affected by this label rendering change
rg -l "FormLabel|nestedFormControl"

# Look for related test files
rg -l "nested.*form.*control.*test"

# Search for form label related code patterns
ast-grep --pattern 'label?.length > 0'

# Look for similar form configurations
rg -A 3 "shouldRenderSubtitle"

Length of output: 3010


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product Task A simple Todo Bug Something isn't working labels Nov 5, 2024
@alex-golovanov
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

github-actions bot commented Nov 5, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11680863566.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37221.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
app/client/src/components/formControls/PaginationControl.tsx (1)

25-29: LGTM! Consider using CSS custom property for border-color.

The updated grid layout with flexible columns and standardized spacing looks good. The separation using border and padding improves visual hierarchy.

Consider extracting the border color to match the spacing variable usage:

- border-top: 1px solid var(--ads-v2-color-border);
+ border-top: var(--ads-v2-border-width-sm) solid var(--ads-v2-color-border);
app/client/src/components/formControls/SortingControl.tsx (1)

74-76: Consider using grid-gap for better maintainability

The changes look good, but consider using the shorthand grid-gap property instead of separate column-gap and row-gap properties for more concise code.

-  column-gap: var(--ads-v2-spaces-4);
-  row-gap: var(--ads-v2-spaces-2);
+  grid-gap: var(--ads-v2-spaces-2) var(--ads-v2-spaces-4);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6660bd9 and aeb3804.

📒 Files selected for processing (4)
  • app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Zone/styles.module.css (0 hunks)
  • app/client/src/components/formControls/PaginationControl.tsx (1 hunks)
  • app/client/src/components/formControls/SortingControl.tsx (2 hunks)
  • app/client/src/components/formControls/WhereClauseControl.tsx (9 hunks)
💤 Files with no reviewable changes (1)
  • app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Zone/styles.module.css
🔇 Additional comments (10)
app/client/src/components/formControls/PaginationControl.tsx (1)

Line range hint 94-96: Verify the usage of customStyles.

The code still spreads customStyles into defaultStyles, but according to the summary, these properties should have been removed.

Let's verify the usage of customStyles in related components:

app/client/src/components/formControls/SortingControl.tsx (2)

57-57: LGTM: Improved styling consistency

The changes properly utilize ADS v2 design system variables for spacing and borders.

Also applies to: 59-60


65-65: LGTM: Consistent spacing

Properly aligned with design system spacing variables.

app/client/src/components/formControls/WhereClauseControl.tsx (7)

Line range hint 93-105: LGTM: Fixed typo in function name

The rename from handleSecondaryBoxBackgroudColor to handleSecondaryBoxBackgroundColor corrects the spelling.


119-121: LGTM: Improved styling consistency

The padding adjustments and border-top addition enhance visual hierarchy and component separation.

Also applies to: 129-132


142-142: LGTM: Adopted design system spacing

Replaced hardcoded gap value with CSS variable for better maintainability.


165-166: LGTM: Standardized grid spacing

Updated grid gaps to use design system spacing variables.


218-220: LGTM: Enhanced responsive layout

Added proper spacing and layout constraints for better responsiveness.


309-312: LGTM: Improved type safety

Removed explicit any type annotation from form values selector.


413-413: LGTM: Enhanced button semantics

Added isIconButton prop for better accessibility and design system compliance.

Copy link

github-actions bot commented Nov 5, 2024

Deploy-Preview-URL: https://ce-37221.dp.appsmith.com

@alex-golovanov alex-golovanov added the ok-to-test Required label for CI label Nov 5, 2024
@alex-golovanov
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

github-actions bot commented Nov 5, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11688611470.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37221.
recreate: .

Copy link

github-actions bot commented Nov 5, 2024

Deploy-Preview-URL: https://ce-37221.dp.appsmith.com

@ankitakinger
Copy link
Contributor

Hey @alex-golovanov,
Can you make these 2 minor changes as well? This will make sure to remove the empty space above pagination field in google sheets:
Screenshot 2024-11-05 at 11 25 52 PM

@alex-golovanov
Copy link
Contributor Author

Hey @alex-golovanov, Can you make these 2 minor changes as well? This will make sure to remove the empty space above pagination field in google sheets: Screenshot 2024-11-05 at 11 25 52 PM

Yep, sure thing

@alex-golovanov
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

Copy link

github-actions bot commented Nov 6, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11699141375.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37221.
recreate: .

Copy link

github-actions bot commented Nov 6, 2024

Deploy-Preview-URL: https://ce-37221.dp.appsmith.com

Copy link

github-actions bot commented Nov 6, 2024

Failed server tests

  • com.appsmith.server.refactors.ce.RefactoringServiceCETest#testRefactorActionName_withInvalidName_throwsError

Copy link
Contributor

@ankitakinger ankitakinger left a comment

Choose a reason for hiding this comment

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

LGTM

@alex-golovanov alex-golovanov added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Nov 6, 2024
@alex-golovanov alex-golovanov merged commit ec246fd into release Nov 6, 2024
74 of 76 checks passed
@alex-golovanov alex-golovanov deleted the fix/37198-pagination-sorting-gaps branch November 6, 2024 14:30
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Nov 20, 2024
…horg#37221)

## Description
Adjusted spacing to match design values for filter/sort/pagination form
controls.

Fixes appsmithorg#37198

## Automation

/ok-to-test tags="@tag.IDE"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/11699699613>
> Commit: cb18e1e
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11699699613&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.IDE`
> Spec:
> <hr>Wed, 06 Nov 2024 08:54:32 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Enhanced layout responsiveness for the `Zone`, `Pagination`,
`Sorting`, and `Where Clause` components.
- Improved clarity and usability of pagination feature in the Google
Sheets plugin.

- **Bug Fixes**
	- Corrected a typo in the `WhereClauseControl` component function name.

- **Style**
- Updated CSS for various components to use CSS variables for consistent
spacing.
	- Removed hardcoded widths for improved flexibility in form controls.
- Added border-radius and adjusted padding for the `SecondaryBox`
component.
	- Streamlined rendering logic for form configuration elements.

- **Refactor**
- Minor refactoring of function names and properties for better clarity
and maintainability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Nov 20, 2024
…horg#37221)

## Description
Adjusted spacing to match design values for filter/sort/pagination form
controls.

Fixes appsmithorg#37198

## Automation

/ok-to-test tags="@tag.IDE"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/11699699613>
> Commit: cb18e1e
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11699699613&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.IDE`
> Spec:
> <hr>Wed, 06 Nov 2024 08:54:32 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Enhanced layout responsiveness for the `Zone`, `Pagination`,
`Sorting`, and `Where Clause` components.
- Improved clarity and usability of pagination feature in the Google
Sheets plugin.

- **Bug Fixes**
	- Corrected a typo in the `WhereClauseControl` component function name.

- **Style**
- Updated CSS for various components to use CSS variables for consistent
spacing.
	- Removed hardcoded widths for improved flexibility in form controls.
- Added border-radius and adjusted padding for the `SecondaryBox`
component.
	- Streamlined rendering logic for form configuration elements.

- **Refactor**
- Minor refactoring of function names and properties for better clarity
and maintainability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working IDE Pod Issues that new developers face while exploring the IDE IDE Product Issues related to the IDE Product ok-to-test Required label for CI Task A simple Todo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AR] Incorrect pagination & sorting gaps
2 participants