Skip to content

fix: Revert "feat: allow multiple lines in alert component"#36643

Merged
albinAppsmith merged 2 commits intoreleasefrom
revert-36629-release-blocker/toast-space
Oct 2, 2024
Merged

fix: Revert "feat: allow multiple lines in alert component"#36643
albinAppsmith merged 2 commits intoreleasefrom
revert-36629-release-blocker/toast-space

Conversation

@albinAppsmith
Copy link
Contributor

@albinAppsmith albinAppsmith commented Oct 2, 2024

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

Tip

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


Wed, 02 Oct 2024 07:27:39 UTC

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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 2, 2024

Walkthrough

The changes introduced in this pull request involve the addition of a new styled component StyledButton in the Toast.styles.tsx file, alongside the removal of the StyledPre component. The StyledToast component has been updated to include a section for CSS variables that define various styling properties. In the Toast.tsx file, the rendering logic has been simplified by directly displaying the content string, eliminating the need for the StyledPre wrapper. The overall functionality of the Toast component remains unchanged.

Changes

File Path Change Summary
.../Toast/Toast.styles.tsx Added StyledButton, removed StyledPre, updated StyledToast with CSS variables.
.../Toast/Toast.tsx Removed import of StyledPre, simplified rendering of content in toast.show.
.../cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec.ts Deleted file containing end-to-end tests for button alert functionality.

Possibly related PRs

Suggested labels

Bug, ok-to-test

Suggested reviewers

  • ApekshaBhosale
  • rishabhrathod01
  • hetunandu

🎉 In the realm of code, a change unfolds,
A StyledButton shines, as the story is told!
With StyledPre now gone, the toast stands tall,
Simplified rendering, a win for us all!
CSS variables dance, in a stylish embrace,
In the world of toast, we find our place! 🌟


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 00fb934 and 598a1d3.

📒 Files selected for processing (3)
  • app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/Button_showAlert_multiline_message_spec.ts (0 hunks)
  • app/client/packages/design-system/ads/src/Toast/Toast.styles.tsx (0 hunks)
  • app/client/packages/design-system/ads/src/Toast/Toast.tsx (2 hunks)
💤 Files with no reviewable 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.styles.tsx
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/Toast/Toast.tsx (3)

7-7: Class, let's examine this import statement modification.

Good job on removing the unused import! It's always important to keep our code tidy and free from unnecessary clutter. Remember, a clean codebase is a happy codebase!


Line range hint 1-74: Class, let's summarize what we've learned from this code review.

The changes we've examined today are in line with our lesson objective of reverting a previous change that caused issues with the toast functionality. We've seen two main modifications:

  1. The removal of an unused import, which helps keep our code clean and efficient.
  2. The simplification of toast content rendering, which addresses the problems introduced in the previous pull request.

These changes demonstrate the importance of code review and the willingness to step back and correct course when necessary. Remember, in the world of coding, as in life, it's okay to admit when something isn't working and to make the necessary adjustments.

For your homework, I want you to think about:

  1. How might these changes improve the overall functionality of the toast component?
  2. What testing strategies would you employ to ensure these changes resolve the issues without introducing new ones?

Class dismissed!


43-43: Now, let's turn our attention to this interesting change in the toast.show method.

Well done on simplifying the content rendering! This change makes the code more straightforward and easier to understand. However, class, can anyone tell me what potential side effect this might have? That's right! By removing the StyledPre component, we might be affecting the styling of our toast content.

Let's make sure we've considered this:

  1. Have we tested this change with various types of content?
  2. Are we confident that the styling remains consistent without the StyledPre wrapper?

Remember, in coding as in life, every action has a reaction. Let's ensure we've thought through all the consequences of our changes.

To verify the impact of this change, let's run a quick check:

✅ Verification successful

Great attention to detail in setting up the verification scripts! I noticed an issue with the second command where 'typescript' isn't recognized as a valid file type by ripgrep. Let's adjust the script to use the correct file types for TypeScript.

Here's the updated script to ensure we accurately check for any remaining StyledPre usages:

Please run this updated script to verify that the removal of StyledPre doesn't inadvertently affect other parts of the codebase.


Excellent effort in refining the verification script! It appears that ripgrep doesn't recognize 'tsx' as a valid file type in our current setup. Let's adjust our approach to ensure we catch all relevant files by specifying their extensions directly.

Please use the updated script below to accurately search for any remaining StyledPre usages:

This adjustment ensures that we search through all TypeScript and JavaScript files by specifying their extensions explicitly. Please run this updated script to verify that the removal of StyledPre doesn't inadvertently impact other parts of the codebase.


Excellent work on simplifying the Toast component's content rendering! The verification confirms that StyledPre is no longer used anywhere else in the codebase, ensuring that its removal doesn't cause any styling inconsistencies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any CSS styles that might have been applied to StyledPre

# Test: Search for any CSS related to StyledPre
rg --type css 'StyledPre'

# Test: Check if there are any other usages of StyledPre that might be affected
rg --type typescript --type javascript 'StyledPre'

Length of output: 111


Script:

# Description: Check for any CSS styles that might have been applied to StyledPre

# Test: Search for any CSS related to StyledPre
rg --type css 'StyledPre'

# Test: Check if there are any other usages of StyledPre that might be affected
rg --type ts --type tsx --type javascript 'StyledPre'

Length of output: 107


Script:

#!/bin/bash
# Description: Check for any CSS styles that might have been applied to StyledPre

# Test: Search for any CSS related to StyledPre
rg --type css 'StyledPre'

# Test: Check if there are any other usages of StyledPre that might be affected
rg 'StyledPre' -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx'

Length of output: 82


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.

@albinAppsmith albinAppsmith changed the title Revert "fix: Release blocker: toast with whitespace" Revert "feat: allow multiple lines in alert component" Oct 2, 2024
@albinAppsmith albinAppsmith added the ok-to-test Required label for CI label Oct 2, 2024
@rishabhrathod01 rishabhrathod01 changed the title Revert "feat: allow multiple lines in alert component" fix: Revert "feat: allow multiple lines in alert component" Oct 2, 2024
@github-actions github-actions bot added the Bug Something isn't working label Oct 2, 2024
@albinAppsmith albinAppsmith merged commit 4f50141 into release Oct 2, 2024
@albinAppsmith albinAppsmith deleted the revert-36629-release-blocker/toast-space branch October 2, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working ok-to-test Required label for CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants