feat: allow multiple lines in alert component #36126
Conversation
saiprabhu-dandanayak
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe changes introduce a new styled component, Changes
Poem
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
|
saiprabhu-dandanayak
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec..ts (1 hunks)
- app/client/packages/design-system/ads/src/Toast/Toast.tsx (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_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.
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec..ts (1)
1-6: Review of imports and object structure.The imports are structured well, and it's good to see that you're using a centralized object repository for your test elements, which is a best practice in maintaining scalable and maintainable test code. Keep up the good work!
app/client/packages/design-system/ads/src/Toast/Toast.tsx (1)
42-42: Excellent use of<pre>for preserving message formatting.The addition of the
<pre>tag withfont: 'inherit'is a clever choice, ensuring that multiline content is displayed correctly while maintaining the inherited font styles. This change effectively addresses the issue of displaying multiline alerts by preserving the formatting and spacing of the content, which is crucial for readability, especially when displaying code snippets or detailed error messages.
| describe( | ||
| "Tests for showAlert message with newline characters in the alert message", | ||
| { tags: ["@tag.Widget", "@tag.Button"] }, | ||
| () => { | ||
| before("Login to the app and navigate to the workspace", function () { | ||
| entityExplorer.DragDropWidgetNVerify(draggableWidgets.BUTTON); | ||
| }); | ||
|
|
||
| it("Verify showAlert message renders correctly with newline characters", () => { | ||
| propPane.EnterJSContext("onClick", "showAlert(`sai\n\nprabhu`)"); | ||
| agHelper.ClickButton("Submit"); | ||
| cy.wait(500); | ||
| cy.get('.Toastify', { timeout: 1000 }) | ||
| .should('have.text', 'sai\n\nprabhu'); | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Review of test structure and logic.
The structure of your test suite is clear and well-organized. The use of tags to categorize the tests is a good practice as it aids in selective test execution and reporting. However, I have a few points to address:
- Avoid using
cy.wait(): Usingcy.wait(500)at line 19 is generally discouraged as it introduces potential flakiness and makes the tests slower. Instead, consider using Cypress's built-in wait mechanisms that wait for specific conditions. - Selector Best Practices: At line 20, you're using a class selector
.Toastify. It's recommended to use data attributes for selectors to make the tests more robust against changes in styling or structure. - Assertion Improvement: The assertion at line 21 checks for exact text, which is good. However, consider enhancing this to check for the presence of newline characters explicitly if that's critical to the test's intent.
Here are some suggestions to improve your test:
- Replace
cy.wait(500)with a more deterministic wait condition. - Use a data attribute for the
.Toastifyselector. - Ensure the assertion explicitly checks for the presence and correct rendering of newline characters.
- cy.wait(500);
+ cy.get('.Toastify', { timeout: 1000 }).should('be.visible');
- cy.get('.Toastify', { timeout: 1000 })
+ cy.get('[data-cy="toast-message"]', { timeout: 1000 })
- .should('have.text', 'sai\n\nprabhu');
+ .should('include.text', 'sai')
+ .and('include.text', 'prabhu');Committable suggestion was skipped due to low confidence.
|
@appsmithorg/query-js-pod , Please review this pr if you have enough bandwidth. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10716935421. |
|
@saiprabhu-dandanayak Could you please fix the lint and prettier checks that are failing for the PR? |
|
Deploy-Preview-URL: https://ce-36126.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec..ts (1 hunks)
- app/client/packages/design-system/ads/src/Toast/Toast.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec..ts
- app/client/packages/design-system/ads/src/Toast/Toast.tsx
|
Hi @rishabhrathod01 , fixed the |
|
Hi @rishabhrathod01 , if you have enough bandwidth , can you pls review this pr , i would be happy to work on comments if any. |
…smith into feat/allow-multiple-lines-in-alert
NilanshBansal
added
Query & JS Pod
|
@carinanfonseca can you please check the design changes here |
|
hello @saiprabhu-dandanayak It makes sense to recognize the \n when we are in JS mode, but in non-JS mode, a user shouldn't be expected to use \n to create a new line. In that mode, pressing 'Enter' in the text area itself should work, like this: Additionally, the spacing between the first and second lines doesn't look correct. It should follow the regular spacing, like this: let me know if this is possible. I am happy to answer any questions. |
|
Hi @carinanfonseca , the screenshot i have added in PR description have double |
|
@saiprabhu-dandanayak could you fix the storybook checks that are failing for the PR? |
|
Hi @rishabhrathod01 , The error is showing it lacks |
|
@saiprabhu-dandanayak you could ignore the storybook failure for now. The Appsmith team will forward the designs for the Toast UI to be implemented here. cc: @carinanfonseca |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11011491433. |
|
Deploy-Preview-URL: https://ce-36126.dp.appsmith.com |
|
@saiprabhu-dandanayak could merge the latest release branch to the PR? Will need trigger the workflow again. |
…smith into feat/allow-multiple-lines-in-alert
|
Hi @rishabhrathod01 , took recent pull from |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_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.
Additional comments not posted (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec.ts (3)
1-6: Good job with the imports, class!You've correctly imported the necessary helper functions from ObjectsCore. This shows good organization and adherence to best practices in our Cypress tests.
8-14: Excellent work on setting up the test suite, students!Your describe block is well-structured with an appropriate description and tags. The before hook efficiently sets up the test environment by dragging and dropping a button widget. Keep up the good work!
1-26: Overall assessment of your work, class:You've done a commendable job creating this test file! It's focused, well-structured, and effectively tests the multiline alert functionality. Here's a summary of your performance:
- Excellent use of helper functions and proper test setup.
- Good adherence to most Cypress best practices.
- Clear and descriptive test case.
To further improve:
- Implement the suggestions from the previous comment to align fully with our coding guidelines.
- Consider adding more assertions to thoroughly validate the alert's appearance and behavior.
Keep up the great work, and remember: practice makes perfect in the world of testing!
.../cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec.ts
Outdated
Show resolved
Hide resolved
.../cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec.ts
Outdated
Show resolved
Hide resolved
…smith into feat/allow-multiple-lines-in-alert
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/packages/design-system/ads/src/Toast/Toast.tsx (2)
43-43: Excellent work on implementing the styled component!You've successfully wrapped the content in a
StyledPrecomponent. This is a great improvement that will allow for better formatting of multi-line messages. However, let's make one small adjustment to improve readability:Consider removing the extra space inside the
StyledPrecomponent:- <StyledPre> {content}</StyledPre> + <StyledPre>{content}</StyledPre>This will ensure that there's no unintended leading space in your formatted content.
43-43: Let's add a helpful note for future reference!Class, while our
StyledPrecomponent will likely preserve newline characters, it would be beneficial to add a comment explaining this behavior. This will help other developers understand the purpose of this change.Consider adding a comment above the
StyledPrecomponent:+ {/* StyledPre preserves newline characters (\n) in the content */} <StyledPre>{content}</StyledPre>This small addition will make your code more informative and easier to maintain in the future. Remember, clear documentation is key to good coding practices!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/packages/design-system/ads/src/Toast/Toast.styles.tsx (1 hunks)
- app/client/packages/design-system/ads/src/Toast/Toast.tsx (2 hunks)
🔇 Additional comments not posted (2)
app/client/packages/design-system/ads/src/Toast/Toast.tsx (1)
7-7: Very good, class! Let's examine the import statement.I see you've added
StyledButtonandStyledPreto your imports. This is a step in the right direction for using styled components in your Toast. Well done!app/client/packages/design-system/ads/src/Toast/Toast.styles.tsx (1)
67-69: Class, let's examine this new addition to our code!Excellent work on introducing the
StyledPrecomponent! This new styled<pre>element will be instrumental in displaying multiline text in our alert messages. By usingfont: inherit;, we ensure that the text maintains visual consistency with its surroundings.Remember, children, in web development,
<pre>stands for "preformatted text." It's like a special notebook that keeps all the spaces and line breaks exactly as we write them. This will be perfect for our multiline alert messages!
|
Test has passed for the shadow PR - #36514 This PR is safe to merge once the EE counterpart passes all the tests |
…smith into feat/allow-multiple-lines-in-alert
There was a problem hiding this comment.
Changes look good to me.
Both CE and EE shadow PR ci-test run has passed.
CE - #36514
EE - https://github.com/appsmithorg/appsmith-ee/pull/5218
Contributor made linting and prettier changes after that and the lint check has passed for the same.
This PR is ready to be merged.
## Description There was an extra space infront of toast content which cause cypress failures. This PR address that issue. PR cause this issue: #36126 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## 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/11121364189> > Commit: a114bff > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11121364189&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 01 Oct 2024 09:28:44 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 - **Chores** - Minor formatting adjustments made to the Toast component. - Added a TODO comment regarding future accessibility features for toasts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This reverts commit a6c109b.
|
@saiprabhu-dandanayak We are reverting this PR since it is causing below issues.
|
Reverts #36629 Reverts #36126 Reverting this PR since it is causing multiple issues in toast and we are reverting the original PR caused this issue. ## 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/11138591480> > Commit: 598a1d3 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11138591480&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 02 Oct 2024 07:27:39 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new styled button component for enhanced toast notifications. - Updated toast styling with customizable properties for better visual presentation. - **Bug Fixes** - Simplified toast body structure by removing unnecessary components, ensuring content displays correctly. - **Chores** - Removed outdated end-to-end test file for button widget alert functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes : #30851
I have raised this pr in-order to allow multiple lines in alert message
Screenshots
passing
\nin the messageunable to parse
\nAfter issue is resolved
Cypress Testing video
Button_showAlert_multiline_message_spec.ts.mp4
Summary by CodeRabbit
New Features
StyledPrefor enhanced formatting in toast notifications.Bug Fixes