Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#12901 closed Bug (fixed)

[Notification aggregator] Not able to change the type of the notification to "success" when process is done

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

When notification is done message should be changed and the type of it (to success). At the moment I can not implement such behavior.

Now documentation suggest that new notification should be created: https://github.com/ckeditor/ckeditor-dev/blob/major/plugins/notificationaggregator/plugin.js#L490-L492 It will work as long as the default notification behavior is used. If user will use notificationShow and notificationUpdate events and cancel default behavior, he will get two notifications instead of one. As I think about notification, which can be shown with some animation (fade in), it would be better if after upload is done the same notification show the information that the progress is done. In fact it is part of the same process, not the separate information. Also getting two notifications for one operation is strange.

No I can prevent default finished behavior, but if I do so I am not able to reset aggregator (I have to use private method) and if I change the type of the notification it does not change its type back when a new task is added.

Change History (7)

comment:1 Changed 4 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned

comment:2 Changed 4 years ago by Piotr Jasiun

Status: assignedreview

Now aggregator does nothing after it is finished and let user decide what he want to do: change the type of the notification to success, hide it or leave it.

I also removed reset method. Lets keep it simple. I believe that one aggregator should have only one notification. When all tasks are done and user want to start counting from the begging he should create a new aggregator instance. There could be some problems with the reset method and as I checked it tests does not covered cases with adding tasks after reset. Eventually we can add reset method in the future.

I have updated code, documentation and tests. Changes in t/12901.

comment:3 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:4 Changed 4 years ago by Piotrek Koszuliński

I pushed branch:t/12901 with a rebased branch + one small commit.

comment:5 Changed 4 years ago by Piotr Jasiun

Status: review_failedreview

My bad. I forgot to push 2 commit. Branch updated.

comment:6 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on major with git:88d5cae.

comment:7 Changed 4 years ago by Piotrek Koszuliński

Version: 4.5.0 (GitHub - major)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy