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

Adjust UX in new folder/file creation dialog #1938

Merged
merged 4 commits into from
Sep 12, 2019
Merged

Adjust UX in new folder/file creation dialog #1938

merged 4 commits into from
Sep 12, 2019

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Sep 9, 2019 •
edited
Loading

Description

  • empty name now shows an error
  • added default value
  • added placeholder text

Related Issue

Motivation and Context

See ticket

How Has This Been Tested?

Manual test:

  • TEST: empty file shows error message
  • TEST: empty file shows placeholder
  • TEST: initial file name is set
  • TEST: empty folder shows error message
  • TEST: empty folder shows placeholder
  • TEST: initial folder name is set

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

Checklist:

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

Open tasks:

  • TODO: fix default file/folder name issue
  • TODO: update acceptance tests

@PVince81 PVince81 requested a review from LukasHirt September 9, 2019 10:14
@PVince81 PVince81 self-assigned this Sep 9, 2019
@PVince81
Copy link
Contributor Author

PVince81 commented Sep 9, 2019

@LukasHirt I'll need your help to debug the value/input value issue.

I tried passing in a default value for the text using either :value or :inputValue but it seems it's not getting set in the dialog. It's always empty. I wonder if there is a mix up between property name and data name.

@PVince81 PVince81 mentioned this pull request Sep 9, 2019
Closed
@PVince81 PVince81 force-pushed the newfiledialog-adjust-ux branch from 7d60057 to 0d072ab Compare September 9, 2019 15:42
@PVince81
Copy link
Contributor Author

PVince81 commented Sep 9, 2019

I've added automated tests

@ownclouders
Copy link
Contributor

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

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

20190909-154335-746.png
20190909-154335-972.png
20190909-154400-367.png
20190909-154400-592.png
20190909-154425-165.png
20190909-154425-346.png
20190909-154448-703.png
20190909-154448-960.png
20190909-154510-495.png
20190909-154533-546.png

@ownclouders
Copy link
Contributor

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

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

20190909-154927-481.png
20190909-154927-596.png
20190909-154946-334.png
20190909-154946-454.png

@ownclouders
Copy link
Contributor

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

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

20190909-154927-374.png
20190909-154927-470.png
20190909-154946-621.png
20190909-154946-687.png

@ownclouders
Copy link
Contributor

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

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

20190909-154626-147.png
20190909-154651-975.png
20190909-154718-540.png
20190909-154747-431.png
20190909-154813-559.png
20190909-154848-675.png
20190909-155000-080.png
20190909-155026-342.png
20190909-155102-266.png
20190909-155128-121.png
20190909-155153-390.png
20190909-155219-215.png

@ownclouders
Copy link
Contributor

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

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

20190909-155017-356.png
20190909-155109-920.png
20190909-155157-375.png
20190909-155245-591.png
20190909-155327-821.png
20190909-155410-426.png

@ownclouders
Copy link
Contributor

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

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

20190909-155431-664.png
20190909-155503-232.png
20190909-155534-896.png
20190909-155606-552.png
20190909-155637-381.png
20190909-155718-844.png
20190909-155751-481.png
20190909-155823-506.png

@ownclouders
Copy link
Contributor

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

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

20190909-160903-292.png
20190909-160934-965.png
20190909-161005-602.png
20190909-161035-890.png
20190909-161107-757.png
20190909-161139-558.png
20190909-161300-479.png
20190909-161331-428.png
20190909-161402-453.png
20190909-161432-082.png
20190909-161520-059.png
20190909-161549-652.png

@PVince81
Copy link
Contributor Author

PVince81 commented Sep 10, 2019 •
edited
Loading

image

but the screenshot does show the alert box ?!

@LukasHirt
Copy link
Collaborator

I'll need your help to debug the value/input value issue.

I'll take a look.

@LukasHirt
Copy link
Collaborator

@PVince81 The problem with the value is that it's not passed as a prop. If you use only :value then you are assigning the value to the dialog container. To assign it to the input you need to create a value prop and then pass it into the input.

@PVince81
Copy link
Contributor Author

create a value prop

@LukasHirt there is already a property called "value". Or do you mean I need to pass it as "value=xxx" without the colon ?

@PVince81
Copy link
Contributor Author

or is the property passing around missing inside the oc dialog prompt maybe ?

@PVince81
Copy link
Contributor Author

  • raise issue for "+ new" button randomly failing to be found because there are two elements with the same id in the DOM

@LukasHirt
Copy link
Collaborator

value=xxx

With this you'd lose the variable and it would write the string directly without translations

or is the property passing around missing inside the oc dialog prompt maybe ?

Yes, the value is in the oc-text-input component but is not being assigned in the dialog component. The input then doesn't know about the value.

@PVince81
Copy link
Contributor Author

@LukasHirt but the "oc-text-input" is using "inputValue" as model and I see it being copied around in the code below the template. I tried evaluating both and none is set from outside despite :value='xxx'

@PVince81
Copy link
Contributor Author

according to https://github.com/owncloud/phoenix/blob/newfiledialog-adjust-ux/apps/files/src/components/ocDialogPrompt.vue#L73 this would copy value to inputValue.

However for some reason this.value is empty despite it being listed as property. This is the part I don't understand well. Maybe setting with ":value" is not the correct way ?

@LukasHirt
Copy link
Collaborator

but the "oc-text-input" is using "inputValue" as model

But since inputValue is not a prop but data you can't the value inside from different component. Unless you'd use mixin which would need changes and wouldn't make much sense to be used here.

@PVince81
Copy link
Contributor Author

@LukasHirt see my last comment. The value is being copied from this.value but this.value is always empty for some reason.

@PVince81
Copy link
Contributor Author

From what I see the rename dialog is receiving a value and it works. I'll check how that one is doing it. Apparently it doesn't use the "value" property directly in the markup.

@PVince81 PVince81 force-pushed the newfiledialog-adjust-ux branch from 5fd395a to 9739443 Compare September 10, 2019 10:17
@PVince81
Copy link
Contributor Author

Default value is now fixed using the same approach as rename dialog.

@PVince81 PVince81 marked this pull request as ready for review September 10, 2019 10:18
@ownclouders
Copy link
Contributor

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

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

20190910-102541-342.png
20190910-102609-882.png

@ownclouders
Copy link
Contributor

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

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

20190910-103017-634.png
20190910-103041-968.png

@ownclouders
Copy link
Contributor

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

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

20190910-102509-472.png
20190910-102544-297.png
20190910-102617-883.png
20190910-102651-816.png
20190910-102725-314.png
20190910-102758-884.png
20190910-102832-425.png
20190910-102906-280.png
20190910-102940-523.png
20190910-103014-684.png
20190910-103055-178.png
20190910-103128-837.png
20190910-103201-979.png
20190910-103235-107.png
20190910-103308-352.png
20190910-103342-719.png
20190910-103445-255.png
20190910-103517-793.png
20190910-103550-599.png
20190910-103631-496.png
20190910-104034-471.png
20190910-104034-781.png
20190910-104110-649.png
20190910-104110-939.png
20190910-104646-689.png
20190910-104729-009.png

@LukasHirt
Copy link
Collaborator

LukasHirt commented Sep 10, 2019 •
edited
Loading

From what I see the rename dialog is receiving a value and it works. I'll check how that one is doing it. Apparently it doesn't use the "value" property directly in the markup.

It takes v-model as a value. If you would want to write directly value without the prop it would need to have input as the root node. Sorry for not mentioning this earlier. It completely slipped my mind.

Vincent Petry added 4 commits September 10, 2019 15:31
Adjust UX in new folder/file creation dialog
a1df595 
- empty name now shows an error
- added default value
- added placeholder text
Make sure the oc-dialog alert prompt can be found
cba338d 
Use class name instead of id for the oc-dialog alert prompt element,
because Selenium would not see the element if there are multiple
elements with the same id, even if the others are invisible.
Trigger input events when clearing folder dialog value
7771bba 
When calling clearValue(), Vue's v-model do not detect any change events
so the "empty folder name" validation fails to trigger.

This adds a workaround with a new custom command clearValueWithEvent()
that will simulate clearing the field with keyboard keys instead of
programmatically.
Wait for new file menu to be enabled before clicking
e300128 
Tests fail randomly when the test runner would click on the "+ new" file
menu before it becomes enabled.

This extends the selector to wait for an enabled button to appear before
clicking.
@PVince81 PVince81 force-pushed the newfiledialog-adjust-ux branch from f6d5115 to e300128 Compare September 10, 2019 13:32
@PVince81
Copy link
Contributor Author

What an adventure! Just dealt with random failures which are fixed now:

@PVince81
Copy link
Contributor Author

This is ready for review

LukasHirt
LukasHirt approved these changes Sep 12, 2019
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.

Looks good 👍

@PVince81
Copy link
Contributor Author

I'd also like to know if my additions to the testing code are ok @individual-it

@PVince81 PVince81 merged commit ccc819e into master Sep 12, 2019
@delete-merged-branch delete-merged-branch bot deleted the newfiledialog-adjust-ux branch September 12, 2019 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@LukasHirt LukasHirt LukasHirt approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@PVince81 PVince81

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[IE11-QA] Missing warning message when File name is empty
4 participants
@PVince81 @ownclouders @LukasHirt @individual-it