-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add more hints to settings #4540
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
Conversation
Signed-off-by: Jan-Christoph Borchardt <[email protected]>
|
@jancborchardt, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @MorrisJobke, @schiessle and @Xenopathic to be potential reviewers. |
|
looks all good 👍 |
| <?php if ($_['outgoingServer2serverShareEnabled']): ?> | ||
| <div id="fileSharingSettings" class="section"> | ||
| <h2><?php p($l->t('Federated Cloud')); ?></h2> | ||
| <p class="settings-hint"><?php p($l->t('You can share with anyone who uses Nextcloud, ownCloud or Pydio! Just put their Federated Cloud ID in the share dialog. It looks like [email protected]')); ?></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well ... if we do this here (Hint to Nextcloud and ownCloud, Pydio) then we need to do that everywhere, don't we? Wouldn't be a more neutral word like "supported federation server" better? 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's tech-speak and unhelpful though. The question is mainly to @karlitschek if we can/should use the word "ownCloud" there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it is fine, but @karlitschek has the last decision here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it is fine of course to show the name of other projects. It is a shared standard after all. But on a different note. I think we should hide product names all together, including Nextcloud, when using branding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should hide product names all together, including Nextcloud, when using branding?
Makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me remove this for now and merge the PR. We should look into the branding of this text in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MorrisJobke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it how it is 👍
jospoortvliet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good.
|
The Samba test failure is unrelated I guess? https://drone.nextcloud.com/nextcloud/server/7557/50 |
| </a> | ||
| <?php endif; ?> | ||
|
|
||
| <?php if (!empty($_['settings-hint'])): ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what description below is for....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So please revert and add the class to the actual description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickvergessen I disagree. We need to separate these into two paragraphs for readability. One is an explanation for what you can use it for, the other is a description for how to use it. The first one is important for anyone who wants to find out if this is useful. The second one we actually could show below the form because it’s only relevant when you decided you want to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jancborchardt here too.
| </a> | ||
| <?php endif; ?> | ||
|
|
||
| <?php if (!empty($_['settings-hint'])): ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So please revert and add the class to the actual description
As afollow-up to #4478, adding more hints to settings.
Text review very welcome @karlitschek @MorrisJobke @schiessle @nickvergessen
More to come for other apps I have locally, but I need to go to sleep …