feat: Action redesign - changes in save and edit datasource #36222
Conversation
…o action-redesign/rest-api
WalkthroughThe changes in this pull request involve updates to user interface messages for editing and saving data sources, simplifying the strings to "Edit" and "Save." Additionally, the icon for the save button in the Changes
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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
IDE Navigation
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10787667465. |
|
Corresponding EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5095 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
app/client/src/components/editorComponents/StoreAsDatasource.tsx (1)
39-39: Consider extracting the function to avoid re-creation on each render.I understand that you've disabled the ESLint rule to allow using a new function as a prop, potentially for performance considerations. However, creating a new function on each render can lead to unnecessary re-renders and performance issues.
Consider extracting the
saveOrEditDatasourcefunction outside the component to avoid re-creation on each render. This way, you can still use it as a prop without the performance overhead.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/src/ce/constants/messages.ts (1 hunks)
- app/client/src/components/editorComponents/StoreAsDatasource.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/ce/constants/messages.ts
Additional comments not posted (1)
app/client/src/components/editorComponents/StoreAsDatasource.tsx (1)
68-68: Great choice of icon for the save action!The "database-2-line" icon is a fitting representation for the save datasource action. It aligns well with the operation being performed and enhances the visual clarity of the button.
The conditional logic to determine the icon based on the
shouldSaveprop is also implemented correctly.
|
Deploy-Preview-URL: https://ce-36222.dp.appsmith.com |
…o action-redesign/rest-api
…o action-redesign/rest-api
albinAppsmith
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Sanity/Datasources/GraphQL_spec.ts (1)
Line range hint
1-1: TODO comment: Add tests.It's a good practice to use TODO comments to keep track of pending tasks. Consider creating a GitHub issue to ensure this task is prioritized and completed.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Sanity/Datasources/GraphQL_spec.ts (2 hunks)
- app/client/cypress/support/Objects/CommonLocators.ts (1 hunks)
- app/client/src/components/editorComponents/StoreAsDatasource.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/components/editorComponents/StoreAsDatasource.tsx
Additional context used
Path-based instructions (2)
app/client/cypress/e2e/Sanity/Datasources/GraphQL_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Objects/CommonLocators.ts (1)
Pattern
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
Additional comments not posted (4)
app/client/cypress/e2e/Sanity/Datasources/GraphQL_spec.ts (3)
282-282: Good practice: UsingagHelper.GetNClickwith a locator variable.The change from
agHelper.ClickButton("Save as datasource")toagHelper.GetNClick(locators._saveDatasource)is a good practice. It enhances the maintainability and readability of the test code by using a locator variable instead of a hard-coded string.
299-299: Verify the assertion change for the visibility of the "Save as datasource" button.The assertion for the visibility of the "Edit datasource" button has been replaced with an assertion for the visibility of the "Save as datasource" button. This change suggests an update in the expected UI behavior after saving a datasource.
Please ensure that this change aligns with the current functionality and verify that the "Save as datasource" button is indeed visible after saving a datasource.
306-306: Consistent usage ofagHelper.GetNClickwith a locator variable.The change from
agHelper.ClickButton("Save as datasource")toagHelper.GetNClick(locators._saveDatasource)is consistent with the previous change and follows the same good practice of using a locator variable for better maintainability and readability.app/client/cypress/support/Objects/CommonLocators.ts (1)
341-341: Good addition:_saveDatasourceproperty in theCommonLocatorsclass.The addition of the
_saveDatasourceproperty to theCommonLocatorsclass is a valuable enhancement. It provides a specific locator for the save datasource button, making it easier to interact with this element in tests.Moreover, using a data-testid selector for the locator is a good practice. It ensures reliable element identification and improves the stability of the tests.
…o action-redesign/rest-api
…org#36222) ## Description This PR updates the save datasource text to "Save" and edit datasource text to "Edit". Also, updated the save datasource icon to better represent datasource. Fixes appsmithorg#35503 ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/10807660093> > Commit: fda9c74 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10807660093&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 11 Sep 2024 09:43:42 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 - **New Features** - Simplified user interface messages for editing and saving data sources to "Edit" and "Save." - Updated the icon for the save button to better represent the action when saving a datasource. - Introduced a new locator for the save datasource button to enhance testing capabilities. - **Bug Fixes** - Improved adherence to coding standards by modifying ESLint rules related to function props. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
This PR updates the save datasource text to "Save" and edit datasource text to "Edit". Also, updated the save datasource icon to better represent datasource.
Fixes #35503
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/10807660093
Commit: fda9c74
Cypress dashboard.
Tags:
@tag.AllSpec:
Wed, 11 Sep 2024 09:43:42 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes