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

Remove confusing placeholder in Sharing Settings #20605

Closed
wants to merge 1 commit into from

Conversation

matthiasbe
Copy link

Hi,
I would like to provide a solution for issue #19806

I am new on this project, as a quick introduction I just installed the server at home and found it wounderful. As I know a bit of web development I would like to help improving it.

Best
Matthias

@gary-kim gary-kim added 3. to review Waiting for reviews bug labels Apr 23, 2020
@gary-kim gary-kim added this to the Nextcloud 19 milestone Apr 23, 2020
@gary-kim
Copy link
Member

@jancborchardt I think you said you'd rather have 7 be a default but do you think this is okay?

This was referenced Apr 23, 2020
@jancborchardt
Copy link
Member

@jancborchardt I think you said you'd rather have 7 be a default but do you think this is okay?

Yeah, I would still say it’s good if there is a default suggestion, and for that to be 7. :)

Otherwise everyone who wants to use this would have to check the checkmark and then put in a number. With a default, if you think 7 is reasonable (which we decided it often is) then it’s just one enabling click.

@rullzer rullzer removed this from the Nextcloud 19 milestone Apr 24, 2020
@matthiasbe
Copy link
Author

If I understand correctly, you want the following

When I first click on "set default expiration date for link shares" I get this
image
Days amount is prefilled with "7" but "Enforce expiration date" is unticked

If I got this right, we run into the problem stated in #19806, analoguous to the current version with the placeholder. It looks like the default value is 7 even though the "7" is just a suggestion.

@jancborchardt
Copy link
Member

If I got this right, we run into the problem stated in #19806, analoguous to the current version with the placeholder. It looks like the default value is 7 even though the "7" is just a suggestion.

The problem in that issue is: The "7" in the field is slightly greyed out and effectively NOT set. – by simply actually using it as the value, we fix the issue. That’s the only thing we need to fix. :)

@matthiasbe
Copy link
Author

@jancborchardt I pushed a new commit according to your indications.

If no value is set and "Set default expiration date for link shares" is ticked, values is set to 7 and updated on the server

I used jQuery's trigger("change") to trigger the updating event.

@jancborchardt
Copy link
Member

If no value is set and "Set default expiration date for link shares" is ticked, values is set to 7 and updated on the server

What happens if you remove the value? It’s just set to 7 again? It’s a bit strange of an interaction – it would be better to just leave the placeholder of "7" there, and if it’s empty use 7 as value. But don’t fill it in the field.

@matthiasbe
Copy link
Author

matthiasbe commented May 10, 2020 via email

@jancborchardt
Copy link
Member

Yup, a placeholder is preferred, as it’s weird behavior to fill a field with a value when people didn’t put it in. :) A placeholder will communicate it much better.

@matthiasbe
Copy link
Author

matthiasbe commented May 10, 2020

@jancborchardt
Ok thank you for the extra precision.
Here is the full behavior:

  • If "Set default expiration date for link shares" is ticked, the value is sent to the server.
  • Whenever the value is empty, 7 is sent to the server

Note that if the box is ticked without any value, and then page is reloaded, the value will be 7 (not the placeholder) because the value is filled on server-side.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Seems good design-wise now. :) cc @skjnldsv for server

@@ -79,15 +79,24 @@ $(document).ready(function(){
value = 'no';
}
}
if ((this.id === 'shareapiExpireAfterNDays' || this.id === 'shareapiInternalExpireAfterNDays') && value === '') {
value = '7'
Copy link
Member

Choose a reason for hiding this comment

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

I guess we have an available value for this somewhere? Hardcoded numbers like those should be avoided

Copy link
Author

Choose a reason for hiding this comment

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

I agree, however I couldn't find any constant for this. Do you have any pointer on where to add this ?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I missed this comment.
Maybe in the capabilities?

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthiasbe any update here? :)

This was referenced May 20, 2021
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@MorrisJobke MorrisJobke mentioned this pull request Jun 10, 2021
59 tasks
This was referenced Jun 16, 2021
@blizzz blizzz modified the milestones: Nextcloud 22, Nextcloud 23 Jun 24, 2021
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv
Copy link
Member

As there is no feedback since a while I will close this PR. If you are still willing to get this in, please address the potential comments and rebase to latest master. Then, feel free to re-open.

@skjnldsv skjnldsv closed this Oct 13, 2021
@skjnldsv skjnldsv removed this from the Nextcloud 23 milestone Oct 13, 2021
@matthiasbe
Copy link
Author

Hello, sorry I didn't give any update. After a while it's hard to get back into the code. Closing is the best thing to do for now, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants