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.

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

[full-ci] Implement copy, cut, paste in ContextMenu | Fix nav icon flickering in lightmode #7309

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Jul 21, 2022

Description

LocationPicker in Contextmenu is now replaced with clipboard actions (as already used with keyboard actions)
See #6892

What needs to be fixed in acceptance tests:

We've replaced the right-click contextmenu actions "copy" and "move", which opened the locationpicker, with clipboard actions. Now we have "cut", "copy" and "paste" instead and they should behave like e.g. in windows explorer.
Therefore we need to adjust how the tests copy or move files since the old way doesn't exist anymore.

Screenshots

Copy/paste keyboard shortcuts in contextmenu
image
Cutting elements "blurs" relevant resources / Paste becomes visible when there is an item in clipboard
image
Paste here button becomes visible if there are items in clipboard
image
Clipboard can be cleared by clicking close on the paste button (which also removes the paste here button)
image

Related Issue

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:

@update-docs
Copy link

update-docs bot commented Jul 21, 2022

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.

@lookacat lookacat changed the title Implement copy, cut, paste | Fix nav icon flickering in lightmode [full-ci] Implement copy, cut, paste | Fix nav icon flickering in lightmode Jul 21, 2022
@ownclouders
Copy link
Contributor

ownclouders commented Jul 21, 2022

Results for oCISTrashbinUploadMoveJourney https://drone.owncloud.com/owncloud/web/27461/70/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIMoveFilesFolders-moveFiles_feature-L106.png

webUIMoveFilesFolders-moveFiles_feature-L106.png

@lookacat lookacat marked this pull request as ready for review July 21, 2022 10:17
@lookacat lookacat changed the title [full-ci] Implement copy, cut, paste | Fix nav icon flickering in lightmode [full-ci] Implement copy, cut, paste in ContextMenu | Fix nav icon flickering in lightmode Jul 26, 2022
@lookacat lookacat force-pushed the implement-context-menu-clipboard-copy-move branch 2 times, most recently from 5fa3fb1 to 44d8507 Compare July 27, 2022 13:18
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

I have mixed feelings about this PR. Feels like really good progress. But technically I don't like to see more and more in the vuex store... 🤔

packages/web-app-files/src/mixins/actions/paste.js Outdated Show resolved Hide resolved
@kulmann
Copy link
Member

kulmann commented Jul 27, 2022

Had a glitch with what seems to be a prefix match issue. When I create two folders with names test and test2 on the same level, then copy test, navigate into test2 and try to paste I get an error message that I can't paste a folder into itself.

@lookacat
Copy link
Contributor Author

lookacat commented Aug 1, 2022

@phil-davis @individual-it could someone from your team please take over required changes in the acceptance tests? :)

@lookacat lookacat force-pushed the implement-context-menu-clipboard-copy-move branch from c0d9edc to 0aae8d5 Compare August 1, 2022 08:20
@SagarGi SagarGi self-assigned this Aug 1, 2022
@SagarGi
Copy link
Member

SagarGi commented Aug 2, 2022

@lookacat @kulmann Some findings with this PR

  1. After copying a specific file and selecting a destination folder to copy we have no cancel options to cancel copy as previouly.

(screenshot from master build)

Screenshot from 2022-08-02 15-37-09

need to know if its a feature or the X works as cancel in here?

  1. And also we cannot copy paste in public link resources
cannotcopyinpubliclink.mp4

needs some information regarding this.

@SagarGi SagarGi force-pushed the implement-context-menu-clipboard-copy-move branch from c372d42 to ec0b016 Compare August 3, 2022 03:51
@SagarGi SagarGi requested a review from kulmann August 3, 2022 07:07
@kulmann
Copy link
Member

kulmann commented Aug 9, 2022

  1. After copying a specific file and selecting a destination folder to copy we have no cancel options to cancel copy as previouly.
    need to know if its a feature or the X works as cancel in here?

we don't need to test the cancel option anymore, as we don't navigate into a different view (location picker is obsolete with this PR). you just stay in the files list of any files related view. As a result you can delete tests that cancel the location picker.

  1. And also we cannot copy paste in public link resources

cannotcopyinpubliclink.mp4

needs some information regarding this.

Needs to be fixed by a dev. Thanks for the pointer!

@kulmann
Copy link
Member

kulmann commented Aug 9, 2022

@SagarGi

I added a fix for your second finding (pasting broken in public links) in 417f875

And in 6a9c79d I removed the location picker entirely, as it is not used anymore in the UI. There are some tests referencing the location picker. Some can be deleted (cancelling copy or move), but for some I'm not sure if you can rewrite them easily (anything that's successful copy/move e.g. from the personal page) or if the time would be better spent writing new ones in the e2e playwright tests and deleting them in the nightwatch test suite. What do you think?

@SagarGi SagarGi force-pushed the implement-context-menu-clipboard-copy-move branch from 6a9c79d to c2ebcec Compare August 10, 2022 04:12
@SagarGi
Copy link
Member

SagarGi commented Aug 10, 2022

@SagarGi

I added a fix for your second finding (pasting broken in public links) in 417f875

And in 6a9c79d I removed the location picker entirely, as it is not used anymore in the UI. There are some tests referencing the location picker. Some can be deleted (cancelling copy or move), but for some I'm not sure if you can rewrite them easily (anything that's successful copy/move e.g. from the personal page) or if the time would be better spent writing new ones in the e2e playwright tests and deleting them in the nightwatch test suite. What do you think?

@kulmann if location picker is entirely removed then tests related to it on (nightwatch) can be removed and time can be spent on e2e playwright for that as you say.

@SagarGi SagarGi force-pushed the implement-context-menu-clipboard-copy-move branch from c2ebcec to c541a9e Compare August 10, 2022 06:45
@SagarGi
Copy link
Member

SagarGi commented Aug 10, 2022

@kulmann i forgot to pull your changes from (#7309 (comment)) to my local and force push it (my very bad). So i have added them with commit 1b8cb5b and c541a9e

@kulmann
Copy link
Member

kulmann commented Aug 10, 2022

@kulmann i forgot to pull your changes from (#7309 (comment)) to my local and force push it (my very bad). So i have added them with commit 1b8cb5b and c541a9e

No worries. Thanks for re-adding them. For future force pushes, just use git push --force-with-lease, that will abort the force push if the remote branch had changes in the meantime. Or just the shorthand gpf if you use https://ohmyz.sh

@kulmann
Copy link
Member

kulmann commented Aug 10, 2022

@SagarGi
I added a fix for your second finding (pasting broken in public links) in 417f875
And in 6a9c79d I removed the location picker entirely, as it is not used anymore in the UI. There are some tests referencing the location picker. Some can be deleted (cancelling copy or move), but for some I'm not sure if you can rewrite them easily (anything that's successful copy/move e.g. from the personal page) or if the time would be better spent writing new ones in the e2e playwright tests and deleting them in the nightwatch test suite. What do you think?

@kulmann if location picker is entirely removed then tests related to it on (nightwatch) can be removed and time can be spent on e2e playwright for that as you say.

Cool. Thank you for already taking care! ❤️ I have removed some more dead code in the nightwatch test suite and moved the paste action from the location picker page object (deleted now) to the files list page object. Hope the test suite becomes green now :-)

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Awesome! 😍 The UX without the location picker is so much better, love it.

@kulmann kulmann force-pushed the implement-context-menu-clipboard-copy-move branch from 6af48f0 to b1af341 Compare August 10, 2022 09:46
@SagarGi
Copy link
Member

SagarGi commented Aug 10, 2022

@SwikritiT @saw-jan @kiranparajuli589 may be someone can review the testscode changes.

@kulmann
Copy link
Member

kulmann commented Aug 10, 2022

@SwikritiT @saw-jan @kiranparajuli589 may be someone can review the testscode changes.

I already did 😅 my review was for code and test-code. Looks all good to me 💪

@kulmann kulmann marked this pull request as draft August 10, 2022 11:38
@kulmann
Copy link
Member

kulmann commented Aug 10, 2022

Temporarily converted back to draft because I want to get #7425 merged first. That one already had too many rebases...

@SagarGi
Copy link
Member

SagarGi commented Aug 10, 2022

@SwikritiT @saw-jan @kiranparajuli589 may be someone can review the testscode changes.

I already did sweat_smile my review was for code and test-code. Looks all good to me muscle

Thanks for if in that case.

@kulmann kulmann force-pushed the implement-context-menu-clipboard-copy-move branch from b1af341 to 75616db Compare August 10, 2022 14:09
@kulmann kulmann force-pushed the implement-context-menu-clipboard-copy-move branch from 75616db to 58d0990 Compare August 10, 2022 14:09
@sonarcloud
Copy link

sonarcloud bot commented Aug 10, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

33.0% 33.0% Coverage
12.5% 12.5% Duplication

@kulmann kulmann marked this pull request as ready for review August 10, 2022 14:17
@kulmann kulmann merged commit cd6b493 into master Aug 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the implement-context-menu-clipboard-copy-move branch August 10, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants