Skip to content

Inform user if the form is anonymous #635

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
Merged
skjnldsv merged 1 commit into from
Oct 13, 2020
Merged

Inform user if the form is anonymous #635

skjnldsv merged 1 commit into from
Oct 13, 2020

Conversation

@Nienzu
Copy link
Member

@Nienzu Nienzu commented Oct 12, 2020 •
edited by skjnldsv
Loading

Fix #530

@Nienzu
Copy link
Member Author

Nienzu commented Oct 12, 2020

Implement #530

skjnldsv
skjnldsv requested changes Oct 12, 2020
View reviewed changes
src/views/Create.vue Outdated
<!-- Check the form's meta information-->
<p v-if="mandatoryUsed" class="info-mandatory">
* {{ t('forms', 'Required questions') }}
<template v-if="isAnonymous">
Copy link
Member

@skjnldsv skjnldsv Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hey!
There is a cleaner way to do that :)

Could you create a computed like infoMessage that returns those strings based on the isAnonymous/mandatoryUsed directly?
That would keep the template clean 😉

Copy link
Member Author

@Nienzu Nienzu Oct 12, 2020

Choose a reason for hiding this comment

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

Thanks for the suggestion!

Copy link
Member Author

@Nienzu Nienzu Oct 12, 2020

Choose a reason for hiding this comment

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

I use v-text to keep the template clean.

@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Oct 12, 2020
@skjnldsv skjnldsv added this to the 2.1 milestone Oct 12, 2020
@Nienzu Nienzu force-pushed the inform_user_anonymous branch from b2fb779 to 929c8fc Compare October 12, 2020 10:14
skjnldsv
skjnldsv reviewed Oct 12, 2020
View reviewed changes
src/views/Create.vue Outdated
Comment on lines 198 to 202
infoMessage() {
const isMandatoryUsed = this.form.questions.reduce(
(isUsed, question) => isUsed || question.mandatory
, false)
if (isMandatoryUsed) {
Copy link
Member

@skjnldsv skjnldsv Oct 12, 2020

Choose a reason for hiding this comment

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

Suggested change
infoMessage() {
const isMandatoryUsed = this.form.questions.reduce(
(isUsed, question) => isUsed || question.mandatory
, false)
if (isMandatoryUsed) {
isMandatoryUsed() {
return this.form.questions.reduce((isUsed, question) => isUsed || question.mandatory, false)
},
infoMessage() {
if (this.isMandatoryUsed) {

src/views/Create.vue Outdated
Comment on lines 202 to 211
if (isMandatoryUsed) {
if (this.form.isAnonymous) {
return t('forms', 'Responses are connected to your Nextcloud account. An asterisk (*) indicates mandatory questions.')
} else {
return t('forms', 'Responses are anonymous. An asterisk (*) indicates mandatory questions.')
}
} else {
if (this.form.isAnonymous) {
return t('forms', 'Responses are connected to your Nextcloud account.')
} else {
return t('forms', 'Responses are anonymous.')
}
}
Copy link
Member

@skjnldsv skjnldsv Oct 12, 2020

Choose a reason for hiding this comment

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

You can use string combination to avoid making translators write 4 different translations :)

Suggested change
if (isMandatoryUsed) {
if (this.form.isAnonymous) {
return t('forms', 'Responses are connected to your Nextcloud account. An asterisk (*) indicates mandatory questions.')
} else {
return t('forms', 'Responses are anonymous. An asterisk (*) indicates mandatory questions.')
}
} else {
if (this.form.isAnonymous) {
return t('forms', 'Responses are connected to your Nextcloud account.')
} else {
return t('forms', 'Responses are anonymous.')
}
}
let message = ''
if (this.form.isAnonymous) {
message += t('forms', 'Responses are connected to your Nextcloud account.')
} else {
message += t('forms', 'Responses are anonymous.')
}
if (this.isMandatoryUsed) {
message += ' ' + t('forms', 'An asterisk (*) indicates mandatory questions.')
}
return message

Also, are you sure about this.form.isAnonymous ?
shouldn't we return Responses are anonymous when it's true instead? Missing a ! ?

Copy link
Member Author

@Nienzu Nienzu Oct 13, 2020

Choose a reason for hiding this comment

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

It's my mistake. Thanks!!

jotoeri
jotoeri reviewed Oct 12, 2020
View reviewed changes
Copy link
Member

@jotoeri jotoeri left a comment

Choose a reason for hiding this comment

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

Nice! 💪
Just one small addition to previous comments. ;)

src/views/Create.vue Outdated
* {{ t('forms', 'Required questions') }}
</p>
<!-- Generate form information message-->
<p class="info-mandatory" v-text="infoMessage" />
Copy link
Member

@jotoeri jotoeri Oct 12, 2020

Choose a reason for hiding this comment

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

Maybe rename also to info-message? Just for consistency...

Copy link
Member Author

@Nienzu Nienzu Oct 13, 2020

Choose a reason for hiding this comment

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

That sounds good. Let's do it!

@Nienzu
Inform user if the form is anonymous
0f91f85 
Signed-off-by: nienzu <ibqqz0602@gmail.com>
@Nienzu Nienzu force-pushed the inform_user_anonymous branch from 929c8fc to 0f91f85 Compare October 13, 2020 02:15
@Nienzu Nienzu requested review from jotoeri and skjnldsv October 13, 2020 02:51
jotoeri
jotoeri approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@jotoeri jotoeri 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 to me 👍 🚀

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Oct 13, 2020
@skjnldsv skjnldsv merged commit 8efb024 into nextcloud:master Oct 13, 2020
@skjnldsv
Copy link
Member

skjnldsv commented Oct 13, 2020

🚀 🎉

@jancborchardt
Copy link
Member

jancborchardt commented Oct 13, 2020

By the way @Nienzu we have a Nextcloud Talk instance for quicker communication among contributors, with channels for e.g. the Forms app, Deck app, Vue team etc. :) If you like we can add you there via the email from your commits?

@Nienzu
Copy link
Member Author

Nienzu commented Oct 14, 2020

@jancborchardt
Yes, it sounds cool~Thanks for your invitation : )

@xadium xadium mentioned this pull request Feb 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skjnldsv skjnldsv skjnldsv approved these changes

@jotoeri jotoeri jotoeri approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request

Projects

None yet

Milestone

2.1

Development

Successfully merging this pull request may close these issues.

Inform user: Is it anonymous?

4 participants

@Nienzu @skjnldsv @jancborchardt @jotoeri