Opened 4 years ago

Closed 3 years ago

#12870 closed New Feature (fixed)

Replace "alert()" with warning notification

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

Description (last modified by Piotr Jasiun)

Since we have notifications we can replace some usage of alert() method with warning notification. I found 3 potential places where we could change it:

Warning notification could be used if notification plugin is loaded and alert function otherwise.

Change History (8)

comment:1 Changed 4 years ago by Piotr Jasiun

Description: modified (diff)
Milestone: CKEditor 4.5.0

comment:2 Changed 4 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.5.0

There is no reason to assign this to the 4.5 though.

comment:3 Changed 4 years ago by Jakub Ś

Status: newconfirmed

comment:4 Changed 3 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:5 Changed 3 years ago by Piotr Jasiun

Milestone: CKEditor 4.5.0
Status: assignedreview

This ticket occurred to be interesting test of the notification API.

First of all it seems to be a good solution to have a method in the editor which show an alert, if there is no notification plugin, and show notification, if there is. Therefore plugins which want just show basic information does not need to require notifications plugin. Editor should know nothing about plugins, so that method should not check if notification plugin is available, but because notification plugin adds editor.showNotification, editor can have default showNotification method which use alert.

Second problem was what kind of notification should be default. My fist approach was to change the default type to warning. It is more similar to alert, needs more attention, does not hide automatically. Also messages used in alert are called error*. But then I realized that we used alert only because we did not have better notification for these cases and they do not need that much attention and are the prefect example for the info notification (when user press "Cut" or "Copy" button on Chrome or Firefox, he should see information, not warning). So I switched back to info as a default notification type.

It turned out that file browser use alert in the dialog, so I left alert there and checked other two plugins.

I pushed changes and manual test to t/12870b. In t/12870 branch you can find warning messages as default, but I believe they looks worse.

Because it is part of the API changes it should goes to 4.5.

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

Status: reviewreview_failed

I pushed rebased branch:t/12870b with an additional commit.

R- because editor.showNotification lacks automated test. Please also remove the manual test which checks a thing that can be checked by an automated test. Don't create manual tests for things that can be so well tested automatically.

comment:7 Changed 3 years ago by Piotr Jasiun

Status: review_failedreview

I have added some unit tests for showNotification to check if it call alert if there is no notification plugin and if it is called in case of errors.

I find manual test useful. Thanks to this test I realized how this alert work in the context and decided to change the notification back to info, from warning. This is a UI feature which can not be checked only by unit tests. Also this manual test show the new feature and since during release developers are not checking tickets anymore (thanks to manual tests), this is good way to show that some editors behavior changed.

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

Resolution: fixed
Status: reviewclosed

Merged to major with git:e112a76.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy