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

Notification component added for popup notifications #3544

Merged
merged 6 commits into from
Aug 12, 2022

Conversation

mavisakalyan
Copy link
Contributor

@mavisakalyan mavisakalyan commented Aug 7, 2022

Describe your changes

  • Added a notification component which appears when you hide or show the UI tools using CTR+H or the hide button in the actions menu.
    NOTICE: This component is now dynamic and can be used from ANYWHERE. Just import "setPopupNotification" hook from the AppContext and whenever you set a message in any use case it will appear and disappear after 5 seconds.

What are the steps for a QA tester to test this pull request?

  • Click the hide button in the actions menu and see the notification appear and disappear after 5 seconds.
  • Same when pressing the CTR+H keys on the keyboard

Issue ticket number and link

Issue: 3394
#3394

Screenshots and/or video

Screen.Recording.2022-08-07.at.17.19.28.mov

Checklist before requesting a review

  • I have performed a self-review of my code
  • I am not adding any irrelevant code or assets
  • I am only including the changes needed to implement the change
  • I have playtested and intentionally tried to find error cases but couldn't

In this case it will show the message how to hide and show the UI tools.
@mavisakalyan mavisakalyan requested review from avaer and lalalune August 7, 2022 13:35
@lalalune
Copy link
Contributor

lalalune commented Aug 7, 2022

Text should be:
"Press CTRL + H to unhide UI"
Should fade out and should for 3 seconds with a .5 second fade

Updated the text, added the fadeout as requested.
@mavisakalyan
Copy link
Contributor Author

@lalalune Made the changes, pleas review and proceed.

src/PopupNotification.jsx Outdated Show resolved Hide resolved
src/PopupNotification.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@avaer avaer left a comment

Choose a reason for hiding this comment

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

Some good ideas, but the code needs cleanup.

@alisaad673
Copy link
Contributor

alisaad673 commented Aug 8, 2022

Works fine functionality wise. Needs conflict resolution

@mavisakalyan
Copy link
Contributor Author

Got it will get right on it.

Added timeout delay constants.
@avaer
Copy link
Contributor

avaer commented Aug 11, 2022

This pr does not follow point 3 on the checklist.

@avaer avaer closed this Aug 11, 2022
@lalalune lalalune reopened this Aug 12, 2022
@lalalune lalalune merged commit 481c021 into master Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants