Skip to content

Conversation

@ivinayakg
Copy link

@ivinayakg ivinayakg commented Jul 18, 2022

Description

I created a system for notifications throughout the react application.
You get two main things from the main file one is a react component which has to be rendered on the top of the stack with given a prop of timeout(time you want to show a notification for) which defaults to 2000ms. Second is a simple javascript function NotificationHandler which can be called from anywhere from the entire app with a data object which would be:-

data = {
  message : string,
  type : string (options are "success" | "warning" | "error")
}

The color of the notification would be based upon the "type" of the data object. right now the color would be shown as the background of the notification but as we decide what kind of icon library or solution we would be using then in the future only the icon with the associated color and message from the data will be rendered. The demo of the feature is as below.

Demo Video

Fixes #30

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Manual Testing
  • Written Tests in Jest

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain: Jest (for react)
  • SDK:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link

@harshith-venkatesh harshith-venkatesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good PR, minor changes are requested

Copy link
Contributor

@Neha Neha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. Can we discuss this over a call?

@ivinayakg ivinayakg force-pushed the feature/notifications-ui branch from 756b7de to 7d48924 Compare July 31, 2022 16:56
@ivinayakg
Copy link
Author

i will re-write the tests, but apart from that I have migrated to Context API have a look
@Neha @pallabez

Copy link
Contributor

@Neha Neha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP review

@ivinayakg ivinayakg force-pushed the feature/notifications-ui branch from dbe95c2 to 4213a19 Compare August 2, 2022 08:14
@Neha
Copy link
Contributor

Neha commented Sep 18, 2022

@ivinayakg could you please resolve the conflict file? So, that we can merge this branch

@ivinayakg ivinayakg requested a review from Neha October 3, 2022 05:52
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.

[UI] Notifications

5 participants