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

Move calendar settings into basic settings #13796

Merged
merged 7 commits into from
Feb 1, 2019
Merged

Conversation

jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Jan 24, 2019

Currently the "Groupware" settings section only has Calendar settings. This doesn’t make a lot of sense. And the sentence below the checkbox says it all:

Please make sure to properly set up the email settings above.

Fixed by moving it to basic settings, and improved the spacing (Also fixes background jobs):
calendar settings basic settings

Please review @nextcloud/designers @karlitschek

@georgehrke
Copy link
Member

👎

#13765 Is gonna add more settings to group ware

@georgehrke
Copy link
Member

On top of that, apps that implement custom Room / Resource backends for calendar scheduling are also supposed to put their UI into the groupware category :)

@juliusknorr
Copy link
Member

Circles was also in there iirc.

@jancborchardt
Copy link
Member Author

@georgehrke @juliushaertl alright, good stuff then! :) Will remove the moving of the Calendar server from my pull request. Still modifying some more stuff. :)

@jancborchardt
Copy link
Member Author

jancborchardt commented Jan 24, 2019

Alright, moved it back, but now I need some help – @georgehrke?

  • The docs page at https://docs.nextcloud.com/server/latest/user_manual/pim/calendar.html is empty. This is also where the first run wizard links to. There should really be some content on there and it seems to have been empty for long? cc @jospoortvliet
  • I doubt I linked the other settings sections correctly. ;) The apps/organization/calendar link, and the email settings link would need to be checked.
  • Instead of just the arrow icon linking to the email server settings, it would be good to link the text "the email server" of the text, but I don’t know how to put a link in that translatable text.

calendar server settings

@karlitschek
Copy link
Member

👍

@georgehrke georgehrke self-assigned this Jan 24, 2019
@georgehrke
Copy link
Member

georgehrke commented Jan 24, 2019

The docs page at https://docs.nextcloud.com/server/latest/user_manual/pim/calendar.html is empty. This is also where the first run wizard links to. There should really be some content on there and it seems to have been empty for long?

It's the correct link, but the documentation is just missing. @MariusBluem What's the status on the calendar doc?

Ref: nextcloud/documentation#127

@georgehrke
Copy link
Member

@jancborchardt Done

@georgehrke georgehrke removed their assignment Jan 24, 2019
@jancborchardt
Copy link
Member Author

Awesome, thanks a lot @georgehrke!

Please review @nextcloud/calendar @nextcloud/designers 🚀

<em>
<?php print_unescaped(str_replace(
[
'{emailopen}',
Copy link
Member Author

Choose a reason for hiding this comment

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

@georgehrke guess this is supposed to be called "linkopen"? Not sure :D

Copy link
Member

Choose a reason for hiding this comment

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

I basically copied it from here: https://github.com/nextcloud/server/blob/master/settings/templates/settings/personal/development.notice.php#L16

{emailopen} will only be visible to the translators anyway.

<p>
<input type="checkbox" name="caldav_send_invitations" id="caldavSendInvitations" class="checkbox"
<?php ($_['send_invitations'] === 'yes') ? print_unescaped('checked="checked"') : null ?>/>
<label for="caldavSendInvitations"><?php p($l->t('Send invitations to attendees')); ?></label>
<br>
<em><?php p($l->t('Please make sure to properly set up the email settings above.')); ?></em>
<em>
<?php print_unescaped(str_replace(
Copy link
Member

Choose a reason for hiding this comment

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

Please use p and use the l10n sprintf method:

p($l->t('%s of %s used', [$_['usage'], $_['total_space']]));

Copy link
Member

Choose a reason for hiding this comment

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

p() will escape <a href="../admin#mail_general_settings">

Copy link
Member

Choose a reason for hiding this comment

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

ah yes, then just the translation ;)

Copy link
Member

Choose a reason for hiding this comment

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

but p will print the text and once it's printed i can't replace {emailopen} or {linkclose} 😉

Copy link
Member

Choose a reason for hiding this comment

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

print_unescaped($l->t(
    'Please make sure to properly set up %sthe email server↗%s.',
    '<a href="../admin#mail_general_settings">',
    '</a>'
));

And I can see %sthe causing issues.
I don't know, the str_replace seemed weird to me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I throw <?php p($l->t('Please make sure to properly set up')) ?> <a href="../admin#mail_general_settings"><?php p($l->t('the email server ↗')) ?></a> in the ring 🤣

Copy link
Member

Choose a reason for hiding this comment

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

I don't know, the str_replace seemed weird to me 😀

As i said above already, I just copied it from https://github.com/nextcloud/server/blob/master/settings/templates/settings/personal/development.notice.php#L16
😉

'<a href="../admin#mail_general_settings">',
'</a>',
],
$l->t('Please make sure to properly set up {emailopen}the email server↗{linkclose}.')
Copy link
Contributor

Choose a reason for hiding this comment

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

There is usually one whitespace before ↗.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that should not be here at all since it’s only used for external links. :)

If we have a verdict on the syntax of the link / translation combination, then it can be fixed in one go.

@@ -30,12 +30,26 @@
?>
<form id="CalDAV" class="section">
<h2><?php p($l->t('Calendar server')); ?></h2>
<p class="settings-hint">Also install the <a target="_blank" href="../apps/office/calendar">Calendar app</a>, or
<a target="_blank" href="<?php p(link_to_docs('user-sync-calendars')) ?>" rel="noreferrer noopener">connect your desktop & mobile for syncing</a>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be translated as well ?

@MariusBluem
Copy link
Member

@georgehrke Same as I‘ve done with the CalDAV/CarDAV sync docs, I will create a PR with the Basic stuff which can be improved later ... End of this week :)

@georgehrke
Copy link
Member

I made the text @jancborchardt added translatable, added a ↗ to the link to the docs and removed the ↗ from the link to the mail settings.

@georgehrke
Copy link
Member

@jancborchardt @skjnldsv @MariusBluem Please check again :)

jancborchardt and others added 7 commits February 1, 2019 10:06
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Feb 1, 2019
@rullzer rullzer merged commit b249c16 into master Feb 1, 2019
@rullzer rullzer deleted the design/settings-caldav branch February 1, 2019 12:00
tcitworld added a commit that referenced this pull request Feb 1, 2019
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit that referenced this pull request Feb 4, 2019
On some instances, users don't know each other and it doesn't makes
sense to expose users as contacts (through Contacts Menu, Calendar
Attendees,…) to everyone.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix and add tests

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Change settings template according to #13796

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Empty system addressbook instead of removing it

This makes Federated Cloud Sharing not fail when synching trusted
servers addressbooks (only synching an empty one, not bumping on an
non-existent one).

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix tests

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
tcitworld added a commit that referenced this pull request Feb 28, 2019
On some instances, users don't know each other and it doesn't makes
sense to expose users as contacts (through Contacts Menu, Calendar
Attendees,…) to everyone.

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix and add tests

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Change settings template according to #13796

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Empty system addressbook instead of removing it

This makes Federated Cloud Sharing not fail when synching trusted
servers addressbooks (only synching an empty one, not bumping on an
non-existent one).

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

Fix tests

Signed-off-by: Thomas Citharel <tcit@tcit.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish design Design, UI, UX, etc. enhancement feature: settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants