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

DApp-1846 Notification toast for Channel notifications. #1875

Merged
merged 12 commits into from
Oct 9, 2024

Conversation

mishramonalisha76
Copy link
Collaborator

@mishramonalisha76 mishramonalisha76 commented Sep 27, 2024

Pull Request Template

Ticket Number

Description

  • Problem/Feature:

Type of Change

  • Bug fix
  • New feature
  • Code refactor
  • Documentation update
  • Other (please describe):

Checklist

  • Quick PR: Is this a quick PR? Can be approved before finishing a coffee.
    • Quick PR label added
  • Not Merge Ready: Is this PR dependent on some other PR/tasks and not ready to be merged right now.
    • DO NOT Merge PR label added

Frontend Guidelines

Build & Testing

  • No errors in the build terminal
  • Engineer has tested the changes on their local environment
  • Engineer has tested the changes on deploy preview

Screenshots/Video with Explanation

  • Before: Explain the previous behavior

  • After: What's changed now

image

Additional Context

Review & Approvals

  • Self-review completed
  • Code review by at least one other engineer
  • Documentation updates if applicable

Notes

@mishramonalisha76 mishramonalisha76 self-assigned this Sep 27, 2024
Copy link

In the src/App.tsx file:

  1. There is a missing closing brace } at the end of the handleJoyrideCallback function.
  2. The dotenv.config() function call should be moved above the const GlobalStyle = createGlobalStyle declaration for proper usage.
  3. The setTimeout function inside handleJoyrideCallback function is missing a closing parenthesis.
  4. There are some commented-out code blocks within the <Joyride> component that should be cleaned up for a cleaner codebase.

In the src/blocks/notification/Notification.tsx file:

  1. The hide method inside the notification object is missing a closing brace } which throws off the syntax.

In the src/common/Common.utils.tsx file:

  1. The setTimeout function inside the handleNotificationToast function is missing proper syntax with a closing parenthesis and brace.

Other than the mentioned issues, the code looks good.

@mishramonalisha76 mishramonalisha76 linked an issue Sep 27, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Sep 27, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-10-09 08:02 UTC

Copy link

All looks good.

@rohitmalhotra1420 rohitmalhotra1420 changed the title Notification toast DApp-1846 Notification toast for Channel notifications. Sep 27, 2024
src/App.tsx Outdated Show resolved Hide resolved
src/blocks/notification/Notification.tsx Outdated Show resolved Hide resolved
src/common/Common.utils.tsx Outdated Show resolved Hide resolved
src/common/components/NotificationOverlay.tsx Outdated Show resolved Hide resolved
src/common/hooks/useStream.tsx Outdated Show resolved Hide resolved
src/common/hooks/useStream.tsx Outdated Show resolved Hide resolved
src/contexts/AppContext.tsx Show resolved Hide resolved
src/common/components/index.ts Outdated Show resolved Hide resolved
src/common/hooks/index.ts Outdated Show resolved Hide resolved
src/hooks/useStream.ts Outdated Show resolved Hide resolved
Copy link

It seems the code is too lengthy to review in one go. Let's start by reviewing the package.json file:

In the package.json file:

  1. In the "scripts" section, there seems to be a typo "vite" in the "dev" script. It may be intended to run "vite" like other scripts.
  2. The use of NODE_OPTIONS environment variable for increasing memory (--max-old-space-size=8192) in the "build:pr:preview" and "build" scripts should be verified to ensure it is necessary and appropriate for the project.
  3. In the "deploy" scripts, the build command (yarn build) is executed before deploying, which should be verified to ensure the build is successful before deploying.
  4. The "lint" script is configured to run eslint for .ts, .tsx files only. If there are other file types in the project that need linting, they should be included.

In the App.tsx file:

  1. The import statement for dotenv at the top of the file should use require('dotenv').config() instead of dotenv.config() because the latter is incorrect.
  2. The createGlobalStyle from styled-components seems to be incomplete in the code snippet provided. It is advisable to review the complete implementation.
  3. The handling logic in the handleJoyrideCallback function seems to contain commented out code that may need to be cleaned up.
  4. The usage of useMemo for spaceUI may need to be reviewed to ensure it is necessary and correctly implemented.
  5. There is a commented-out code block that is waiting for resolution on an issue. It should be monitored and cleaned up once resolved.

In the Notification.tsx file under src/blocks/notification/:

  1. In the CloseButton styled div, there seems to be a syntax error with the CSS var(--surface-transparent). It should be checked and corrected.
  2. The implementation of notification.hide(); in the handleNotificationClose function should be reviewed to ensure it functions as intended.
  3. The usage of <>{overlay}</> seems incomplete and may need further context to understand its purpose.

Let's continue reviewing the rest of the files. If I missed something or you need further assistance, feel free to ask.

Copy link

github-actions bot commented Oct 3, 2024

All looks good.

Copy link

github-actions bot commented Oct 7, 2024

All looks good.

Copy link

github-actions bot commented Oct 7, 2024

All looks good.

Copy link

github-actions bot commented Oct 7, 2024

All looks good.

Copy link

github-actions bot commented Oct 7, 2024

All looks good.

Copy link

github-actions bot commented Oct 8, 2024

All looks good.

@rohitmalhotra1420 rohitmalhotra1420 merged commit 628ea2e into main Oct 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement in app channel notifications on Dapp
2 participants