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

Display changes in file versions tab view and detailsView #26511

Merged
merged 3 commits into from Nov 2, 2016

Conversation

Projects
None yet
5 participants
@mjobst-necls
Contributor

mjobst-necls commented Oct 29, 2016

Description

I made some changes to improve the file version tab.

Related Issue

Issue Tag: #26510

Motivation and Context

This pull requests contains code to view the version file size on the file version tab.
Another little change was done for the favourite "star" button on the details view. The opacity of this button changes to 1 on mouse over by this pull request.

How Has This Been Tested?

The test was ok for the following browsers:

  • Internet Explorer 8 on Windows 7
  • Google Chrome (current version) and Mozilla Firefox (current version) on Ubuntu 16.04

Screenshots (if appropriate):

screenshot from 2016-10-29 21-51-33

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.
@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Oct 29, 2016

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Oct 29, 2016

CLA assistant check
All committers have signed the CLA.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 29, 2016

@mjobst-necls, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @schiessle and @icewind1991 to be potential reviewers.

@mjobst-necls, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @schiessle and @icewind1991 to be potential reviewers.

@PVince81 PVince81 added this to the 9.2 milestone Nov 2, 2016

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol Nov 2, 2016

Contributor

Using Chrome Version 54.0.2840.71 (64-bit) in OSX

PR applied:
screen shot 2016-11-02 at 10 49 38

PR not applied:
screen shot 2016-11-02 at 10 50 25

Contributor

davitol commented Nov 2, 2016

Using Chrome Version 54.0.2840.71 (64-bit) in OSX

PR applied:
screen shot 2016-11-02 at 10 49 38

PR not applied:
screen shot 2016-11-02 at 10 50 25

@PVince81

Great PR, love it! Also works well.

There are a few minor issues, please see my review comments.

Would you mind adding the size field in the JS unit tests here https://github.com/owncloud/core/blob/v9.1.1/apps/files_versions/tests/js/versionstabviewSpec.js#L80 ?

Show outdated Hide outdated apps/files_versions/css/versions.css
}
.versionsTabView .version-details > .size {
vertical-align: super;

This comment has been minimized.

@PVince81

PVince81 Nov 2, 2016

Member

Not sure why "super" here ?
On Chromium for me the size appears under the date but not as superscript. It does look good already, so not sure why. If you wanted to shift it up a bit you could try vertical-align: middle and set a different line-height.

@PVince81

PVince81 Nov 2, 2016

Member

Not sure why "super" here ?
On Chromium for me the size appears under the date but not as superscript. It does look good already, so not sure why. If you wanted to shift it up a bit you could try vertical-align: middle and set a different line-height.

Show outdated Hide outdated apps/files_versions/js/versionstabview.js
'<img class="preview" src="{{previewUrl}}"/>' +
'<a href="{{downloadUrl}}" class="downloadVersion"><img src="{{downloadIconUrl}}" />' +
'<span class="versiondate has-tooltip" title="{{formattedTimestamp}}">{{relativeTimestamp}}</span>' +
'<li data-revision="{{timestamp}}">' +

This comment has been minimized.

@PVince81

PVince81 Nov 2, 2016

Member

Please use tabs instead of spaces in all the changed lines.

@PVince81

PVince81 Nov 2, 2016

Member

Please use tabs instead of spaces in all the changed lines.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 2, 2016

Member

Internet Explorer 8

By the way, since 9.2 OC only supports IE >= 11 so no need to test in much older versions 😄

Member

PVince81 commented Nov 2, 2016

Internet Explorer 8

By the way, since 9.2 OC only supports IE >= 11 so no need to test in much older versions 😄

@PVince81 PVince81 added the enhancement label Nov 2, 2016

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol Nov 2, 2016

Contributor

BTW also tested in Safari Version 9.1.2 (10601.7.7) and Firefox 49.0.2 and it went fine

Contributor

davitol commented Nov 2, 2016

BTW also tested in Safari Version 9.1.2 (10601.7.7) and Firefox 49.0.2 and it went fine

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol Nov 2, 2016

Contributor

Another little change was done for the favourite "star" button on the details view. The opacity of this button changes to 1 on mouse over by this pull request.

It works but maybe ideally it would be great that the star will turn into yellow on mouse over as it happens in the files view. With this PR, the star keeps in grey. BTW it is a better behaviour than before. Great job!!

starlight

Contributor

davitol commented Nov 2, 2016

Another little change was done for the favourite "star" button on the details view. The opacity of this button changes to 1 on mouse over by this pull request.

It works but maybe ideally it would be great that the star will turn into yellow on mouse over as it happens in the files view. With this PR, the star keeps in grey. BTW it is a better behaviour than before. Great job!!

starlight

@mjobst-necls

This comment has been minimized.

Show comment
Hide comment
@mjobst-necls

mjobst-necls Nov 2, 2016

Contributor

Using Chrome Version 54.0.2840.71 (64-bit) in OSX

Can please someone tell me why chrome displays code different on OSX than on other OSs. Do you guys know a possibility how to test the code on OSX without apple hardware? I use a vm for tests on windows.

It works but maybe ideally it would be great that the star will turn into yellow on mouse over as it happens in the files view. With this PR, the star keeps in grey. BTW it is a better behaviour than before. Great job!!

Thank you, I'm going to create another PR to cover your change requests regarding the color of the favorite star on hover and the JS unit test enhancements.

Contributor

mjobst-necls commented Nov 2, 2016

Using Chrome Version 54.0.2840.71 (64-bit) in OSX

Can please someone tell me why chrome displays code different on OSX than on other OSs. Do you guys know a possibility how to test the code on OSX without apple hardware? I use a vm for tests on windows.

It works but maybe ideally it would be great that the star will turn into yellow on mouse over as it happens in the files view. With this PR, the star keeps in grey. BTW it is a better behaviour than before. Great job!!

Thank you, I'm going to create another PR to cover your change requests regarding the color of the favorite star on hover and the JS unit test enhancements.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 2, 2016

Member

@mjobst-necls a separate PR for the favorite star color is fine.

However could you add the JS unit test extension to this PR to make it complete since this enhancement is mostly about the versions tab ?

Member

PVince81 commented Nov 2, 2016

@mjobst-necls a separate PR for the favorite star color is fine.

However could you add the JS unit test extension to this PR to make it complete since this enhancement is mostly about the versions tab ?

versions tab enhancements
enhanced js test file
removed css superscript attribute for version size
@mjobst-necls

This comment has been minimized.

Show comment
Hide comment
@mjobst-necls

mjobst-necls Nov 2, 2016

Contributor

Ok, commit done.

Contributor

mjobst-necls commented Nov 2, 2016

Ok, commit done.

Show outdated Hide outdated apps/files_versions/js/versionstabview.js
return _.extend({
formattedTimestamp: OC.Util.formatDate(timestamp),
relativeTimestamp: OC.Util.relativeModifiedDate(timestamp),
humanReadableSize: OC.Util.humanFileSize(size, true),
altSize: n('files', '%n byte', '%n bytes', size),
hasDetails: version.has('size'),

This comment has been minimized.

@PVince81

PVince81 Nov 2, 2016

Member

seems you missed some tabs here

@PVince81

PVince81 Nov 2, 2016

Member

seems you missed some tabs here

This comment has been minimized.

@mjobst-necls

mjobst-necls Nov 2, 2016

Contributor

omg ^^. you're right ....

@mjobst-necls

mjobst-necls Nov 2, 2016

Contributor

omg ^^. you're right ....

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 2, 2016

Member

Thanks. Apart from the missed tabs this looks good.

Member

PVince81 commented Nov 2, 2016

Thanks. Apart from the missed tabs this looks good.

@PVince81 PVince81 merged commit f8ed8fb into owncloud:master Nov 2, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Scrutinizer No new issues
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 2, 2016

Member

👍 merged, thanks a lot!

Added to features page: https://github.com/owncloud/core/wiki/ownCloud-9.2-Features

Member

PVince81 commented Nov 2, 2016

👍 merged, thanks a lot!

Added to features page: https://github.com/owncloud/core/wiki/ownCloud-9.2-Features

elie195 added a commit to elie195/core that referenced this pull request Mar 23, 2017

Display changes in file versions tab view and detailsView (#26511)
* Display changes in file versions tab view and detailsView

* versions tab enhancements

enhanced js test file
removed css superscript attribute for version size

* Replaced spaces with tabs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment