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.

Sign up for GitHub

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

Jump to bottom

feat: add notification center for composer (QnA url import) #4080

Merged
merged 25 commits into from
Sep 25, 2020
Merged

feat: add notification center for composer (QnA url import) #4080

merged 25 commits into from
Sep 25, 2020

Conversation

lei9444
Copy link
Contributor

@lei9444 lei9444 commented Sep 10, 2020 •
edited
Loading

Description

The notification flow is base on @vivekkshankar 's design
https://www.figma.com/file/LlTlh5vXwq91zjGnFtrUUR6l/Composer-main-design-spec-%2F-UI-library?node-id=313%3A15006

  1. support default cards
  2. support customized notification cards
  3. Add time management for every single card.

Task Item

refs #3872

Screenshots

notifications2

@coveralls
Copy link

coveralls commented Sep 10, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.08%) to 55.739% when pulling f777c90 on lei9444:notification into ad90706 on microsoft:main.

@cwhitten
Copy link
Member

@lei9444 @GeoffCoxMSFT I'd like wire in the existing error tooltip that hangs off of the start bot button into this notifications module. We don't need to block this PR on doing that but I'd like it in R11 so we can remain consistent.

@vivekkshankar @sangwoohaan I assume the above is the expectation from a UX side.

@hibrenda
Copy link
Contributor

Seems too short for error notification. After this pop up disappear, user can not find the error message.

@lei9444
Copy link
Contributor Author

lei9444 commented Sep 14, 2020

Seems too short for error notification. After this pop up disappear, user can not find the error message.

I removed the time for error notification, customers can close it by click the close button

@boydc2014 boydc2014 added Approved to merge approved, waiting to be merged and removed Needs review labels Sep 16, 2020
@a-b-r-o-w-n a-b-r-o-w-n removed the Approved to merge approved, waiting to be merged label Sep 16, 2020
@lei9444
Copy link
Contributor Author

lei9444 commented Sep 18, 2020 •
edited
Loading

Hi @hatpick, Thanks for you suggestions and it's very helpful for me. I have updated it.

hatpick
hatpick previously approved these changes Sep 18, 2020
View reviewed changes
@boydc2014 boydc2014 added the Approved to merge approved, waiting to be merged label Sep 24, 2020
@cwhitten cwhitten merged commit 8ad58bd into microsoft:main Sep 25, 2020
@vivekkshankar
Copy link

@cwhitten Missed your message earlier. We do not plan to merge validation errors with notifications.
Notifications are reserved on system progress, publish, runtime errors etc.

The interaction model for validations is a bit different by design.

https://www.figma.com/file/deFhMPBnRWxTYiYDYqYxrc/Composer-Vivek?node-id=8206%3A0

@cwhitten
Copy link
Member

@vivekkshankar you're talking about something different. I do mean the existing runtime & publish errors that we show currently below the start button in a popover element. It would certainly be a bad UX to preserve this & the notification panel.

alanlong9278 added a commit to alanlong9278/BotFramework-Composer that referenced this pull request Sep 28, 2020
@alanlong9278
Merge branch 'main' into julong/zoom-in-out
bb271cb 
* main:
  fix: Monaco editor links opened in blank window in electron (microsoft#4269)
  feat: add notification center for composer (QnA url import) (microsoft#4080)
  fix: Object examples not properly displayed as placeholders (microsoft#4126)
  fix: allows spaces in bot project path (microsoft#4260)
  chore: extract build logic from components page (microsoft#4153)
@cwhitten cwhitten mentioned this pull request Nov 13, 2020
Merged
@lei9444 lei9444 deleted the notification branch February 1, 2021 02:08
benbrown pushed a commit to benbrown/BotFramework-Composer that referenced this pull request May 24, 2021
@alanlong9278
Merge branch 'main' into julong/zoom-in-out
4c23967 
* main:
  fix: Monaco editor links opened in blank window in electron (microsoft#4269)
  feat: add notification center for composer (QnA url import) (microsoft#4080)
  fix: Object examples not properly displayed as placeholders (microsoft#4126)
  fix: allows spaces in bot project path (microsoft#4260)
  chore: extract build logic from components page (microsoft#4153)
benbrown pushed a commit that referenced this pull request Jun 11, 2021
@alanlong9278
Merge branch 'main' into julong/zoom-in-out
899f653 
* main:
  fix: Monaco editor links opened in blank window in electron (#4269)
  feat: add notification center for composer (QnA url import) (#4080)
  fix: Object examples not properly displayed as placeholders (#4126)
  fix: allows spaces in bot project path (#4260)
  chore: extract build logic from components page (#4153)
lei9444 added a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
@lei9444 @boydc2014 @cwhitten
feat: add notification center for composer (QnA url import) (microsof…
69dd86d 
…t#4080)

* feat: add notification center for composer(qna import)

* update some style

* update the style

* update the icon color

* fix conflict

* remove the timer when error

* update the notification create flow

* remove the return in dispatcher

* use typs instead of interface

* use atomFamily to avoid over rendering

* update the set

Co-authored-by: Dong Lei <donglei@microsoft.com>
Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwhitten cwhitten cwhitten approved these changes

@hatpick hatpick hatpick approved these changes

@beyackle beyackle Awaiting requested review from beyackle

@boydc2014 boydc2014 Awaiting requested review from boydc2014

@srinaath srinaath Awaiting requested review from srinaath

@tonyanziano tonyanziano Awaiting requested review from tonyanziano

@GeoffCoxMSFT GeoffCoxMSFT Awaiting requested review from GeoffCoxMSFT

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n

Assignees
No one assigned
Labels
Approved to merge approved, waiting to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lei9444 @coveralls @cwhitten @hibrenda @vivekkshankar @hatpick @a-b-r-o-w-n @boydc2014