Solves issue #26749 - UPDATED #27155

Merged
merged 17 commits into from Feb 23, 2017

Projects

None yet

3 participants

@noveens
Contributor
noveens commented Feb 14, 2017

Description

I replaced all instances of showTemporary with show which has a cross to remove notifications.

Related Issue

#26749

Motivation and Context

Brings a cross to remove pop-up notifications.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • 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 change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@mention-bot

@noveens, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bartv2, @jmaciasportela, @PVince81 and @Xenopathic to be potential reviewers.

@noveens noveens changed the title from 26749 final to Solves issue #26749 - UPDATED Feb 14, 2017
@PVince81
Collaborator

Now thinking of it, we probably don't need the timeout at all for error messages.

The timeout was there in the past because the user could not close error messages. But now with the button they can. And it's better that they stay until closed to make sure the user saw it.

Imagine you start uploading a huge file, go away. Then there is an error. If the error disappears after 7 seconds, you'll never know whether the upload succeeded or not.

So let's remove the timeout for all errors that are closeable.

Just make a single commit, no need to split into so many small commits, there is no benefit for splitting this way. Thanks.

@noveens
Contributor
noveens commented Feb 15, 2017

@PVince81 ,
Sure I'll update the changes by tonight

@noveens noveens timeout removed since there is a button to close it
646cff8
@noveens
Contributor
noveens commented Feb 15, 2017 edited

@PVince81 ,
Updated, please recheck.

apps/files/js/file-upload.js
@@ -547,7 +547,7 @@ OC.Uploader.prototype = _.extend({
},
showUploadCancelMessage: _.debounce(function() {
- OC.Notification.showTemporary(t('files', 'Upload cancelled.'), {timeout: 10});
+ OC.Notification.show('Upload cancelled.', {type: 'error'});
@PVince81
PVince81 Feb 16, 2017 Collaborator

Please leave the timeout for this one. Cancelling an upload is a user action.
I tested this and it feels a bit annoying to have to not only cancel the upload but also close the error.

apps/files/js/filelist.js
- 'Could not rename "{fileName}", it does not exist any more',
- {fileName: oldName}
- )
+ OC.Notification.show('Could not rename "{fileName}", it does not exist any more',
@PVince81
PVince81 Feb 16, 2017 Collaborator

You missed/killed the translation function t() here and many other places.
Please recheck all locations.

apps/files/js/files.js
+ {
+ usedSpacePercent: usedSpacePercent,
+ owner: ownerDisplayName,
+ owner: ownerDisplayName,
@PVince81
PVince81 Feb 16, 2017 Collaborator

duplicate key ? also you removed the translation function

@noveens
noveens Feb 16, 2017 Contributor

I couldn't find the meaning of t() function

Can you please tell me what it does.

I'll update the changes by tonight

@PVince81
PVince81 Feb 16, 2017 Collaborator

t('Some message {placeholder}', {placeholder: 'some value to insert'}) => this will render the message "Some message some value to insert".
The "t" function will translate to another language if you switch to another language in the settings, try it.

It would be good if you could also test the changes you made or at least a few, this way you could see that something is wrong with the code.

@noveens
noveens Feb 16, 2017 edited Contributor

@PVince81 ,
when i made these changes, always a " [ object object ] " message pops up
my guess would be that "showTemporary" did something with that array but "show" doesn't.

@noveens
noveens Feb 16, 2017 Contributor

I have uploaded a screenshot of the same:

http://pasteboard.co/z2viGllSc.png

@PVince81
PVince81 Feb 16, 2017 Collaborator

Have a closer look at the parenthesis. Now you mixed up the values in a different way.

The correct way is:

OC.Notifications.show(
    t(
        'some message maybe with {placeholder}',
        {placeholder: 'value for placeholder'}
    ),
    {
        type: 'error'
    }
);

(placeholder optional, only when applicable)

@noveens
Contributor
noveens commented Feb 16, 2017 edited

@PVince81 ,
Oh got it now, I found some weird dictionarys while randomly going through the code.

Now I know what it does.
Will include the translate capability.

Sure, will test the changes I made.

@noveens noveens translation capability added
dd96bab
@noveens
Contributor
noveens commented Feb 16, 2017

@bartv2, @jmaciasportela, @PVince81 and @Xenopathic ,

Please review the updates, I think bug's solved now.

apps/files/js/filelist.js
@@ -1569,7 +1569,7 @@
if (e instanceof DOMException) {
console.error(e);
this.changeDirectory('/');
- OC.Notification.showTemporary(t('files', 'Invalid path'));
+ OC.Notification.show(t('files', 'Invalid path'), {meout: 7, type: 'error'});
@PVince81
PVince81 Feb 17, 2017 Collaborator

"meout" ?

@noveens
noveens Feb 17, 2017 Contributor

oh sorry, my bad :p

@noveens noveens typo fixed
02ab2f6
@noveens
Contributor
noveens commented Feb 17, 2017

@PVince81 ,

Changes pushed.

@PVince81
Collaborator

Now run make test-js and fix the test issues one by one.

In most cases you need to replace the stubbed call with show instead of showTemporary.
Let me know if you need guidance there.

@noveens noveens test files updated
ab46686
@noveens
Contributor
noveens commented Feb 20, 2017

@PVince81 ,
Thanks for the pointers :P
Updated the test-files now.

@noveens
Contributor
noveens commented Feb 20, 2017

@PVince81 ,
Jenkins still seems to fail, I have no idea why.
Can you please look into it?

@PVince81
Collaborator

@noveens this time it's bad luck:

15:09:53 Fire up the mysql docker
15:09:53 docker: Error response from daemon: grpc: the connection is unavailable.
@noveens
Contributor
noveens commented Feb 20, 2017

@PVince81 ,
anything else required for this issue?

apps/files/js/filelist.js
@@ -2650,7 +2656,7 @@
*/
_showPermissionDeniedNotification: function() {
var message = t('core', 'You don’t have permission to upload or create files here');
- OC.Notification.showTemporary(message);
+ OC.Notification.show(t('files', message), {type: 'error'});
@PVince81
PVince81 Feb 20, 2017 Collaborator

message is already translated, no need to retranslate it here

apps/files/js/file-upload.js
self.cancelUploads();
} else {
// HTTP connection problem or other error
- OC.Notification.showTemporary(data.errorThrown, {timeout: 10});
+ OC.Notification.show(t('files', data.errorThrown), {type: 'error'});
@PVince81
PVince81 Feb 20, 2017 Collaborator

don't translate data.errorThrown here

{
- isHTML: true
+ isHTML: true,
+ type: 'error'
@PVince81
PVince81 Feb 20, 2017 Collaborator

Not technically an error but more like an information message.

Adding the error type does make it display the cross though.

@noveens
noveens Feb 20, 2017 Contributor

I guess we should let this be?

@PVince81
PVince81 Feb 20, 2017 Collaborator

Yes, leave it like it was before for now

@noveens noveens small errors fixed
aebc5e9
@noveens
Contributor
noveens commented Feb 23, 2017 edited

@PVince81 ,
Can this be merged, I think it's fine now.

@PVince81
Collaborator

Yes, Jenkins is green.

@PVince81 PVince81 merged commit 4632201 into owncloud:master Feb 23, 2017

3 of 4 checks passed

continuous-integration/jenkins/pr-head This commit is being built
Details
Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment