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

Add cancel / create button on fileMenu form #39056

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Add cancel / create button on fileMenu form #39056

merged 2 commits into from
Jul 28, 2021

Conversation

AlexAndBear
Copy link

@AlexAndBear AlexAndBear commented Jul 26, 2021 •
edited
Loading

Description

Enhancement: Show create and cancel buttons in the 'New file menu'

If the user hits the '+' button in the UI, a context menu will be shown,
where the user is able to choose for example 'Directory'.
A form will show up to set the new directory name, the user needs to confirm
by hitting the enter key.
This might not been understood by every user at the first glance.
Therefore, with this PR a create and cancel button has been added.

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

2021-07-26_23-58-13 (1)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@AlexAndBear AlexAndBear changed the title Enterprise/issues/4684 Add cancel / create button on fileMenu form Jul 26, 2021
@AlexAndBear
Copy link
Author

AlexAndBear commented Jul 26, 2021 •
edited
Loading

TODO:

  • As enter confirms form, escape should close the form
  • Check with multiple form items (create dir, text file ....)
  • Add Acceptance tests
  • Add JS tests
  • Add changelog item

@AlexAndBear AlexAndBear requested a review from tbsbdr July 26, 2021 12:40
@tbsbdr
Copy link

tbsbdr commented Jul 26, 2021

some visual guidance would be nice, i.e. visualize "create" as the primary action and cancel as the secondary action. I could not find a secondary-styled button in oc classic, but in the "create link" dialog we have a blue primary button which would have the same effect.

--> could you use the blue styling for the create-button?

image

@AlexAndBear
Copy link
Author

@tbsbdr i already updated this pr with blue confirm button

@tbsbdr
Copy link

tbsbdr commented Jul 26, 2021

Works like a charm, thanks for the fix!
2021-07-26_23-58-13 (1)

@AlexAndBear AlexAndBear marked this pull request as ready for review July 27, 2021 09:07
@AlexAndBear
Copy link
Author

@mmattel at first glance, we don't need a docs issue

@AlexAndBear
Copy link
Author

@phil-davis should we create acceptance tests or are the js tests already satisfying ?

@phil-davis
Copy link
Contributor

@janackermann the current acceptance tests "hit enter" to create a folder (or a text file).
We should add a scenario that clicks the "Create" button. And a scenario that clicks the "Cancel" button.

Do you want to do that? Or will I get someone to do it?

(Also, I see that the "Text File" option also has the new Cancel and Create buttons. Similar scenarios could be added to files_texteditor to test those)

@owncloud owncloud deleted a comment from update-docs bot Jul 27, 2021
@AlexAndBear
Copy link
Author

AlexAndBear commented Jul 27, 2021 •
edited
Loading

Do you want to do that? Or will I get someone to do it?

We can do it.

(Also, I see that the "Text File" option also has the new Cancel and Create buttons. Similar scenarios could be added to files_texteditor to test those)

From technically POV this is core functionality and behaves the same on files_texteditor and so on, IHMO no separated tests for apps needed

@AlexAndBear AlexAndBear force-pushed the enterprise/issues/4684 branch 4 times, most recently from 8c5b9fb to ee1d0f7 Compare July 27, 2021 13:32
@phil-davis
Copy link
Contributor

Fails make test-php-style
https://drone.owncloud.com/owncloud/core/31563/2/7

@JammingBen
Copy link
Contributor

JammingBen commented Jul 27, 2021 •
edited
Loading

Question is why, locally everything works fine 🤔

Edit: Actually, no, the test-php-style-fix does not complain. test-php-style does.

@phil-davis
Copy link
Contributor

The extra style checker for the acceptance test code does not have an automatic fixer.

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiCapabilities-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31575/55/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiProvisioningGroups-v2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31575/62/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiAuthOcs-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31575/53/1

@JammingBen
Copy link
Contributor

Okay we really need to fix #39046, this starts to get annoying...

@phil-davis
Copy link
Contributor

Looks good - you could squash the 8 commits.

@AlexAndBear
Copy link
Author

Thanks, squashed into two, for easy patching

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline cliBackground-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/119/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiWebdavLocks2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/94/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiAuthWebDav-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/54/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline cliProvisioning-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/122/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiProvisioning-v2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/60/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiMain-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/58/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiTranslation-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/87/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiSharees-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/67/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiWebdavUpload1-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/103/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiShareManagementBasicToShares-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/71/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiWebdavProperties2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/102/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiWebdavPreviews-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/100/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiSharePublicLink1-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/76/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiShareOperationsToShares2-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/75/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiShareReshareToShares3-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/83/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-10.6.0-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/115/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToRoot1-git-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/107/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToShares1-latest-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/114/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline cliMain-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/121/1

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline cliAppManagement-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31597/124/1

tbsbdr
tbsbdr approved these changes Jul 28, 2021
View reviewed changes
Copy link

@tbsbdr tbsbdr left a comment

Choose a reason for hiding this comment

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

lgtm - thanks!!

@sonarcloud
Copy link

sonarcloud bot commented Jul 28, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

77.8% 77.8% Coverage
0.0% 0.0% Duplication

@AlexAndBear AlexAndBear merged commit 4954f73 into master Jul 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the enterprise/issues/4684 branch July 28, 2021 10:35
@phil-davis phil-davis mentioned this pull request Jul 29, 2021
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-davis phil-davis phil-davis approved these changes

@tbsbdr tbsbdr tbsbdr approved these changes

Assignees

@AlexAndBear AlexAndBear

@JammingBen JammingBen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexAndBear @tbsbdr @phil-davis @JammingBen @ownclouders