Skip to content

Choose button remains disabled unless files are chosen#25900

Merged
PVince81 merged 1 commit into
owncloud:masterfrom
sharidas:choose-btn-disabled-16106
Aug 26, 2016
Merged

Choose button remains disabled unless files are chosen#25900
PVince81 merged 1 commit into
owncloud:masterfrom
sharidas:choose-btn-disabled-16106

Conversation

@sharidas

Copy link
Copy Markdown
Contributor

from filepicker

This is will help user to understand that you can only
click choose button once the files are selected from the
file picker. This addresses the solution for issue 16106

Signed-off-by: Sujith Haridasan Sujith_Haridasan@mentor.com

@mention-bot

Copy link
Copy Markdown

@sharidas, thanks for your PR! By analyzing the annotation information on this pull request, we identified @tanghus, @DeepDiver1975 and @butonic to be potential reviewers

@CLAassistant

CLAassistant commented Aug 22, 2016

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@DeepDiver1975 DeepDiver1975 added this to the 9.2 milestone Aug 22, 2016
Comment thread core/js/oc-dialogs.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please limit this to the scope to the one of the dialog.
If there is any other unrelated DOM element with the class "primary" in the same page, they'd be selected as well.

@PVince81

PVince81 commented Aug 22, 2016

Copy link
Copy Markdown
Contributor
  • TEST: not selecting anything has the button disabled
  • TEST: selecting something enables the button
  • TEST: select a file, then enter a folder, click button 🚫 FAIL
  • BUG: The button must re-disable itself after entering a folder, because it would clear the selection and clicking "Confirm" makes it invalid

@sharidas

Copy link
Copy Markdown
Contributor Author

@PVince81 Thanks for the review. Sure I will make the changes accordingly for the comments shared. Also I will take care of disabling choose button when a folder/directory is selected.

@sharidas sharidas force-pushed the choose-btn-disabled-16106 branch from 57a8b91 to 77faa1c Compare August 23, 2016 06:38
Comment thread core/js/oc-dialogs.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not add an explicit id to the button which allows us to address it directly?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because it means you need to invent very long ids that are unique across pages and across apps.
So here you'd call it (let me exagerate a bit for fun): "ownCloudCoreTheMightyFilePickersPrimaryButton".

Also, if you happen to have two dialogs open at the same time (for some reason, stacked), you'd still have a name conflict.

Best is to simply use scoped selectors. In this case here, self.$filePicker.find('.primary') already does the job nicely.

@sharidas sharidas force-pushed the choose-btn-disabled-16106 branch from 77faa1c to 49e199a Compare August 23, 2016 11:02
Comment thread core/js/oc-dialogs.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no need for $() around getOcDialog. The method closest as for most jQuery methods already return a jQuery object.

@sharidas sharidas force-pushed the choose-btn-disabled-16106 branch from 49e199a to b72ce8d Compare August 24, 2016 10:05
Comment thread core/js/oc-dialogs.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here, you can remove $(), it's already a jQuery object

@PVince81

Copy link
Copy Markdown
Contributor

@sharidas just ping me in the PR when you're done, I'll get an email 😄

@sharidas

Copy link
Copy Markdown
Contributor Author

@PVince81 Thanks for the review feedback. I missed out the $() part near the buttonEnableDisable variable.

@sharidas sharidas force-pushed the choose-btn-disabled-16106 branch from b72ce8d to 29491b4 Compare August 25, 2016 05:03
@sharidas

Copy link
Copy Markdown
Contributor Author

@PVince81 I have updated my patch by removing the $() from the buttonEnableDisable variable too.

@PVince81

Copy link
Copy Markdown
Contributor

Retested and I found another case where the button needs to re-disable itself: when clicking on the breadcrumbs to navigate back to a previous folder.

@PVince81

Copy link
Copy Markdown
Contributor

@sharidas steps to reproduce on this branch:

  1. Create a folder "sub"
  2. Upload pictures in the root and also in "sub"
  3. Go to the personal page
  4. In the avatar section, click the folder icon to bring up the file picker
  5. Click on the "sub" folder, see that the "Choose" button gets enabled
  6. On the breadcrumb at the top of the dialog, click on the house icon

Expected: "Choose" button disabled again
Actual: "Choose" button is still enabled even though nothing is selected

@sharidas

Copy link
Copy Markdown
Contributor Author

@PVince81 I am not able to understand what is going on wrong at my end. For some reason I am seeing choose button disabled : http://imgur.com/a/m9Vau.

@PVince81

Copy link
Copy Markdown
Contributor

@sharidas this looks correct so far since you didn't select anything. Now from here, select something, the button will enable itself. And then click on the house icon. The button must disable again but doesn't.

from filepicker

This is will help user to understand that you can only
click choose button once the files are selected from the
file picker. This addresses the solution for issue 16106

Signed-off-by: Sujith Haridasan <Sujith_Haridasan@mentor.com>
@sharidas sharidas force-pushed the choose-btn-disabled-16106 branch from 29491b4 to 00fb145 Compare August 26, 2016 11:27
@sharidas

Copy link
Copy Markdown
Contributor Author

@PVince81 I have tried with the new change and I have verified by clicking the house icon mentioned in the previous comment. It worked for me. I have also verified by creating different folders inside sub. Hope this sounds ok.

@PVince81

Copy link
Copy Markdown
Contributor

I deleted and checked out the branch again, it works now. Maybe I had an older version, sorry.

👍

@PVince81 PVince81 merged commit 187b3df into owncloud:master Aug 26, 2016
@DeepDiver1975

Copy link
Copy Markdown
Member

Fixes #16106

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants