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

Added new UI for notification settings page #1826

Merged
merged 13 commits into from
Sep 9, 2024

Conversation

abhishek-01k
Copy link
Collaborator

@abhishek-01k abhishek-01k commented Aug 23, 2024

Pull Request Template

Ticket Number

#1785

Description

New UI for Notification settings page.

  • 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

https://www.figma.com/design/FvJL0JWHlsVohsHTERB1Lh/Push-Design-Foundations?node-id=6658-9557&m=dev

  • Before: Old UI

  • After: New UI according to the figma above

Screenshot 2024-08-23 at 6 23 15 AM

Additional Context

Review & Approvals

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

Notes

You should have a channel. Click on Add or Manage notification settings and then you will see the new UI for the notification settings page.

Things left:

  • Validations for the Add Notification settings page
  • Responsiveness for the Add Notification/Edit Notification settings Modal

Copy link

All looks good.

Copy link

github-actions bot commented Aug 23, 2024

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

@rohitmalhotra1420
Copy link
Collaborator

rohitmalhotra1420 commented Aug 26, 2024

@abhishek-01k

  • Click on add Setting and just type in the setting name and hit save settings
  • Now click on the edit setting icon and you will see that Set as default Switch is set to selected.

@rohitmalhotra1420
Copy link
Collaborator

@abhishek-01k no validations applied on the settings name. I click on Save settings button without entering the name, it doesn't throws a validation error and closes the dialog.

@rohitmalhotra1420
Copy link
Collaborator

Screenshot 2024-08-26 at 5 05 15 PM Checkbox color is not correct.

@rohitmalhotra1420
Copy link
Collaborator

@abhishek-01k I think there should be some kind of validations on these fields.

  • Let's say the upper ranger should be always greater than the lower range.
  • If user turns on the Range switch then he should set the range. Right now I can close the dialog by clicking on Save settings with both upper and lower range set to 0
Screenshot 2024-08-26 at 5 05 45 PM

@rohitmalhotra1420
Copy link
Collaborator

Screenshot 2024-08-26 at 5 08 12 PM

The Ux is not correct. When my wallet is not connected and I click on the approve push button then it shows me an empty Alert component.

Copy link

All looks good.

Copy link

All looks good.

Copy link

All looks good.

Copy link

All looks good.

Copy link

github-actions bot commented Sep 3, 2024

I found some issues in the code:

  1. In the Pencil component, there is a syntax error. The path element is not properly formatted:

    • Change:
      <path
        d="M8.47489 20.25H4.46739C4.27712 20.25 4.09465 20.1744 3.96012 20.0399C3.82558 19.9053 3.75 19.7229 3.75 19.5326V15.5251C3.75009 15.3351 3.82555 15.1529 3.95984 15.0185L15.0183 3.95995C15.1529 3.82552 15.3353 3.75 15.5254 3.75C15.7156 3.75 15.898 3.82552 16.0325 3.95995L20.04 7.96476C20.1745 8.09928 20.25 8.28168 20.25 8.47186C20.25 8.66204 20.1745 8.84444 20.04 8.97896L8.98154 20.0402C8.84711 20.1744 8.66489 20.2499 8.47489 20.25Z"
        stroke="currentColor"
        stroke-width="1.5"
        stroke-linecap="round"
        stroke-linejoin="round"
      />
        d="M12.3589 6.61963L17.3806 11.6413"
    • To:
      <path
        d="M8.47489 20.25H4.46739C4.27712 20.25 4.09465 20.1744 3.96012 20.0399C3.82558 19.9053 3.75 19.7229 3.75 19.5326V15.5251C3.75009 15.3351 3.82555 15.1529 3.95984 15.0185L15.0183 3.95995C15.1529 3.82552 15.3353 3.75 15.5254 3.75C15.7156 3.75 15.898 3.82552 16.0325 3.95995L20.04 7.96476C20.1745 8.09928 20.25 8.28168 20.25 8.47186C20.25 8.66204 20.1745 8.84444 20.04 8.97896L8.98154 20.0402C8.84711 20.1744 8.66489 20.2499 8.47489 20.25Z"
        stroke="currentColor"
        strokeWidth="1.5"
        strokeLinecap="round"
        strokeLinejoin="round"
      />
  2. In the TextInput component, there is a missing closing curly brace and parenthesis:

    • Add a closing curly brace } after the description element and a closing parenthesis ) after the ErrorMessage element.
  3. In the CheckboxInput component, the CSS pseudo-class :checked is not being used correctly. It should be placed inside the style definition of the CheckboxInput styled component:

    • Change:
      &:checked {
        accent-color: #C742DD;
      }
    • To:
      & {
        &:checked {
          accent-color: #C742DD;
        }
      }
  4. In the Common form file, there is a typo in the import of Common types:

    • Change:
      import { ModalResponse } from 'common/Common.types';
    • To:
      import { ModalResponse } from './Common.types';
  5. In the useModal hooks file, the import of Common types needs to be corrected as well:

    • Change:
      import { ModalResponse } from 'common/Common.types';
    • To:
      import { ModalResponse } from '../../common/Common.types';

After making these changes, the code should be good to go.

src/common/Common.form.ts Outdated Show resolved Hide resolved
src/common/hooks/useModal.ts Outdated Show resolved Hide resolved
src/common/hooks/index.ts Outdated Show resolved Hide resolved
src/modules/notifSettings/EditNotificationSetting.form.tsx Outdated Show resolved Hide resolved
src/queries/queryKeys.ts Outdated Show resolved Hide resolved
src/pages/NotificationSettingsPage.tsx Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Sep 3, 2024

All looks good.

Copy link

github-actions bot commented Sep 3, 2024

All looks good.

Copy link

github-actions bot commented Sep 5, 2024

All looks good.

@rohitmalhotra1420
Copy link
Collaborator

@abhishek-01k UI is breaking
Screenshot 2024-09-06 at 4 53 31 PM

@rohitmalhotra1420
Copy link
Collaborator

@abhishek-01k I have already added a multi-range setting. Now if I edit and set the value on range toggle switch to false I cannot save the setting. The save button doesn't responds.

Copy link

github-actions bot commented Sep 9, 2024

I have reviewed the provided code. Here are the findings:

  1. In src/blocks/icons/components/Pencil.tsx:
  • There is a syntax error in the SVG path element, the closing angle bracket is missing before the 'd' attribute.
  • The 'stroke-width' attribute should be 'strokeWidth' in camelCase.
  • The 'stroke-linecap' attribute should be 'strokeLinecap' in camelCase.
  • The 'stroke-linejoin' attribute should be 'strokeLinejoin' in camelCase.
  1. In src/blocks/modal/Modal.tsx:
  • The closing tags for and are missing.
  • The 'LabelContainer', 'InputText', 'LabelContainer', and 'InputContainer' styled components are used but not defined in the code snippet provided.
  1. The code in src/common/Common.form.ts, src/common/Common.types.ts, src/common/index.ts, and src/common/hooks/useDisclosure.ts looks correct and has no issues.

  2. The files src/components/reusables/checkbox/Checkbox.tsx, src/components/reusables/sliders/InputSlider.tsx, src/components/reusables/sliders/RangeSlider.tsx, src/modules/channelDashboard/ChannelDashboard.types.ts, src/modules/channelDashboard/components/ChannelDashboardNullState.tsx, src/modules/notifSettings/EditNotificationSetting.form.tsx, src/modules/notifSettings/NotificationSettings.constants.ts, src/modules/notifSettings/NotificationSettings.tsx, src/modules/notifSettings/NotificationSettings.types.ts, src/modules/notifSettings/components/AddNotificationSettingsModalContent.tsx, src/modules/notifSettings/components/NotificationSettingsComponent.tsx, src/modules/notifSettings/components/NotificationSettingsFooter.tsx, src/modules/notifSettings/components/NotificationSettingsHeader.tsx, src/modules/notifSettings/components/NotificationSettingsListItem.tsx, src/modules/notifSettings/components/NotificationSettingsLists.tsx, src/modules/notifSettings/components/NotificationSettingsModal.tsx, src/modules/notifSettings/components/NotificationSettingsModalWrapper.tsx, src/modules/notifSettings/components/NotificationSettingsRangeSelector.tsx, src/pages/NotificationSettingsPage.tsx, src/queries/hooks/index.ts, and src/queries/hooks/notificationSettings/index.ts are empty or not provided for review.

Please make the necessary corrections based on the feedback provided. Let me know if you need further assistance.

Copy link

github-actions bot commented Sep 9, 2024

All looks good.

Copy link

github-actions bot commented Sep 9, 2024

All looks good.

Copy link

github-actions bot commented Sep 9, 2024

All looks good.

@rohitmalhotra1420 rohitmalhotra1420 merged commit a2ca195 into main Sep 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.

😈 [Feature Enhancement] - New UI for Notification settings Page and Modal
2 participants