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

Removed favorite button from file list and added it instead in action… #3336

Merged
merged 1 commit into from
May 14, 2020
Merged

Removed favorite button from file list and added it instead in action… #3336

merged 1 commit into from
May 14, 2020

Conversation

Julian1998
Copy link
Contributor

@Julian1998 Julian1998 commented Apr 14, 2020 •
edited
Loading

… dropdown menu as well as in the file sidebar

Description

Related Issue

Motivation and Context

UI enhancement

How Has This Been Tested?

  • toggle favorite file(s) and folder(s) using the new action in the dropdown menu
  • toggle favorite files using the functionality in the file sidebar menu

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)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@Julian1998 Julian1998 added the Status:Needs-Review Needs review from a maintainer label Apr 14, 2020
@Julian1998 Julian1998 self-assigned this Apr 14, 2020
@update-docs
Copy link

update-docs bot commented Apr 14, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Julian1998
Julian1998 commented Apr 14, 2020
View reviewed changes
apps/files/src/fileactions.js Outdated
{
icon: 'star',
handler: this.toggleFileFavorite,
ariaLabel: this.$gettext('Mark/Unmark as favorite'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukasHirt Any idea how to just use 'mark as favorite' or 'Unmark as favorite'. I couldn't find any possibility to access the file.starred value in this context

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could transform ariaLabel into a method which accepts resource as an argument. Same as is done with isEnabled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem: When trying to do that the function gets interpreted as string value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch 2 times, most recently from aeb4d54 to fcb6799 Compare April 14, 2020 08:00
@Julian1998 Julian1998 requested review from kulmann and removed request for DeepDiver1975 April 14, 2020 08:02
@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch from fcb6799 to 357a629 Compare April 14, 2020 08:07
@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9378/

20200414-083401-925.png
20200414-083514-347.png

DeepDiver1975
DeepDiver1975 previously requested changes Apr 14, 2020
View reviewed changes
apps/files/src/fileactions.js Outdated
@@ -125,7 +146,7 @@ export default {
})
},
reallyDeleteFiles () {
let files = this.deleteDialogSelectedFiles ? this.deleteDialogSelectedFiles : this.selectedFiles
let files = this.dfavoriteFileeleteDialogSelectedFiles ? this.deleteDialogSelectedFiles : this.selectedFiles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidental paste ... @Julian1998

@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch 2 times, most recently from 8562092 to e0bfc72 Compare April 14, 2020 09:37
@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch from e0bfc72 to 1c84829 Compare April 14, 2020 09:43
@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch from 1c84829 to 580f5ff Compare April 14, 2020 12:01
@Julian1998 Julian1998 requested a review from LukasHirt April 14, 2020 13:50
@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch from 580f5ff to e42a3a9 Compare April 14, 2020 14:09
@LukasHirt
Copy link
Collaborator

Could you pls create also mark/unmark as favourite tests in the sidebar?

@PVince81
Copy link
Contributor

PVince81 commented Apr 23, 2020 •
edited by Julian1998
Loading

@Julian1998 any update ?

@Julian1998
Copy link
Contributor Author

Julian1998 commented Apr 24, 2020 •
edited
Loading

I usually work on Fridays. I will finish this tomorrow. (I accidentally edited this answer in your comment above)

@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch 3 times, most recently from 86047ee to 266dba2 Compare April 27, 2020 06:51
LukasHirt
LukasHirt approved these changes Apr 27, 2020
View reviewed changes
Copy link
Collaborator

@LukasHirt LukasHirt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Fix failing tests and I'd say it's ready to be merged.

@Julian1998
Copy link
Contributor Author

LGTM. Fix failing tests and I'd say it's ready to be merged.

Thanks. Yeah I am still on the failing tests.... They are really kind of weird :/

@Julian1998 Julian1998 force-pushed the feature/remove-favorite-button-from-file-list branch from 266dba2 to afab7a8 Compare April 27, 2020 10:21
@LukasHirt
Copy link
Collaborator

@individual-it I've adjusted the tests a bit in the last commit. Could you pls review it?

LukasHirt
LukasHirt reviewed Apr 28, 2020
View reviewed changes
tests/acceptance/features/webUIFavorites/favoritesFile.feature
@@ -14,33 +14,28 @@ Feature: Mark file as favorite
Scenario: mark files as favorites
When the user marks file "data.tar.gz" as favorite using the webUI
And the user marks file "data.zip" as favorite using the webUI
Then file "data.tar.gz" should be marked as favorite on the webUI
And file "data.zip" should be marked as favorite on the webUI
When the user reloads the current page of the webUI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed to refresh because the star is not present anyway.

@LukasHirt LukasHirt force-pushed the feature/remove-favorite-button-from-file-list branch from 626a4f1 to 9d143b7 Compare April 29, 2020 09:22
@LukasHirt
Copy link
Collaborator

@individual-it I've brought back the tests in the WebUI as discussed. It's squashed into the last commit. Pls, re-review.

LukasHirt
LukasHirt reviewed Apr 29, 2020
View reviewed changes
tests/acceptance/features/webUIFavorites/unfavoriteFile.feature
And the following files have been deleted by user "user1"
| name |
| lorem.txt |
| simple-folder |
When the user unmarks the favorited file "lorem.txt" using the webUI
Then no message should be displayed on the webUI
# Then the error message with header 'Error while marking "lorem.txt" as favorite' should be displayed on the webUI
And file "lorem.txt" should not be marked as favorite on the webUI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the unfavorite action doesn't go through we need to expect that the star in the sidebar is still active. At least until we fix the original issue.

@LukasHirt LukasHirt force-pushed the feature/remove-favorite-button-from-file-list branch from 9d143b7 to 903e177 Compare May 3, 2020 19:47
@individual-it individual-it self-requested a review May 4, 2020 07:53
individual-it
individual-it requested changes May 4, 2020
View reviewed changes
tests/acceptance/pageObjects/FilesPageElement/appSideBar.js Outdated Show resolved Hide resolved
tests/acceptance/pageObjects/FilesPageElement/filesRow.js
@@ -3,60 +3,6 @@ const filesList = client.page.FilesPageElement.filesList()

module.exports = {
commands: {
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are getting to the point that the fileRow does not do anything much by itself, need to think if we want to refactor that and get rid of that PageObject (not in this PR)

tests/acceptance/stepDefinitions/filesContext.js Outdated Show resolved Hide resolved
@LukasHirt
Copy link
Collaborator

@individual-it Comments addressed. Ready for another review 😉

@LukasHirt LukasHirt force-pushed the feature/remove-favorite-button-from-file-list branch from 82bdf04 to e305b67 Compare May 12, 2020 08:54
individual-it
individual-it approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests look good

@LukasHirt
Copy link
Collaborator

tests look good

Thanks. I'll try to investigate why it's failing now and then will rebase

@Julian1998 @LukasHirt
Removed favorite button from file list and added it instead in action…
5bc85e6 
… dropdown

Co-authored-by: Julian Müller <[email protected]>
Co-authored-by: Lukas Hirt <[email protected]>
@LukasHirt LukasHirt force-pushed the feature/remove-favorite-button-from-file-list branch from e305b67 to 5bc85e6 Compare May 13, 2020 10:20
@LukasHirt LukasHirt dismissed DeepDiver1975’s stale review May 14, 2020 08:07

Since it was only about a typo, I am going to dismiss this review not to delay this PR.

@LukasHirt LukasHirt merged commit e2502e9 into master May 14, 2020
@LukasHirt LukasHirt deleted the feature/remove-favorite-button-from-file-list branch May 14, 2020 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasHirt LukasHirt LukasHirt approved these changes

@individual-it individual-it individual-it approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@kulmann kulmann Awaiting requested review from kulmann

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@Julian1998 Julian1998

Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove favorite button from file list
6 participants
@Julian1998 @ownclouders @LukasHirt @PVince81 @DeepDiver1975 @individual-it