-
Notifications
You must be signed in to change notification settings - Fork 127
feat: rating question type #3637
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -101,6 +101,7 @@ class Constants { | |||||||||||||||||
| public const ANSWER_TYPE_MULTIPLE = 'multiple'; | ||||||||||||||||||
| public const ANSWER_TYPE_MULTIPLEUNIQUE = 'multiple_unique'; | ||||||||||||||||||
| public const ANSWER_TYPE_RANKING = 'ranking'; | ||||||||||||||||||
| public const ANSWER_TYPE_RATING = 'rating'; | ||||||||||||||||||
| public const ANSWER_TYPE_SHORT = 'short'; | ||||||||||||||||||
| public const ANSWER_TYPE_TIME = 'time'; | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -121,6 +122,7 @@ class Constants { | |||||||||||||||||
| self::ANSWER_TYPE_MULTIPLE, | ||||||||||||||||||
| self::ANSWER_TYPE_MULTIPLEUNIQUE, | ||||||||||||||||||
| self::ANSWER_TYPE_RANKING, | ||||||||||||||||||
| self::ANSWER_TYPE_RATING, | ||||||||||||||||||
| self::ANSWER_TYPE_SHORT, | ||||||||||||||||||
| self::ANSWER_TYPE_TIME, | ||||||||||||||||||
| ]; | ||||||||||||||||||
|
|
@@ -219,6 +221,17 @@ class Constants { | |||||||||||||||||
| 'rows' => ['array'], | ||||||||||||||||||
| ]; | ||||||||||||||||||
|
|
||||||||||||||||||
| /** | ||||||||||||||||||
| * A rating is a linear scale that always starts at 1 and is drawn as icons, so it | ||||||||||||||||||
| * shares the linear scale's key for its top end (and that key's bounds) rather than | ||||||||||||||||||
| * having one of its own. optionsLowest is deliberately absent: a rating's lowest end | ||||||||||||||||||
| * is always 1. ratingIcon is one of 'star' (default), 'heart' or 'thumb'. | ||||||||||||||||||
| */ | ||||||||||||||||||
| public const EXTRA_SETTINGS_RATING = [ | ||||||||||||||||||
|
Comment on lines
+224
to
+230
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need the doc block here
Suggested change
|
||||||||||||||||||
| 'optionsHighest' => ['integer', 'NULL'], | ||||||||||||||||||
| 'ratingIcon' => ['string', 'NULL'], | ||||||||||||||||||
| ]; | ||||||||||||||||||
|
|
||||||||||||||||||
| public const EXTRA_SETTINGS_RANKING = [ | ||||||||||||||||||
| 'shuffleOptions' => ['boolean'], | ||||||||||||||||||
| ]; | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -633,18 +633,23 @@ public function validateSubmission(array $questions, array $answers, string $for | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if all answers are within the possible options | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // A rating carries no options, so it cannot go through the predefined-option | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // branch below, but its answer is a point on a scale exactly as a linear scale's | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // is, so it is held to the same rule. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if ($question['type'] === Constants::ANSWER_TYPE_RATING) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($answers[$questionId] as $answer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->validateScaleAnswer($question, $answer); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED) && empty($question['extraSettings']['allowOtherAnswer'])) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
633
to
645
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. During the review I noticed that we actually shouldn't keep linearscale in the predefined questions array in Constants.php. As a preparation for a follow-up PR we should already move rating questions to a new type array ANSWER_TYPES_SCALE in Constants and check the question type for being a member of that new array.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // Normalize option IDs once for consistent comparison (DB may return ints, request may send strings) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $optionIds = $this->normalizeOptionIds($question['options'] ?? []); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($answers[$questionId] as $answer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Handle linear scale questions | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if ($question['type'] === Constants::ANSWER_TYPE_LINEARSCALE) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $optionsLowest = $question['extraSettings']['optionsLowest'] ?? 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $optionsHighest = $question['extraSettings']['optionsHighest'] ?? 5; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!ctype_digit((string)$answer) || intval($answer) < $optionsLowest || intval($answer) > $optionsHighest) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new \InvalidArgumentException(sprintf('The answer for question "%s" must be an integer between %d and %d.', $question['text'], $optionsLowest, $optionsHighest)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $this->validateScaleAnswer($question, $answer); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if all grid rows, columns and values match the configured grid subtype | ||||||||||||||||||||||||||||||||||||||||||||||||||
| elseif ($question['type'] === Constants::ANSWER_TYPE_GRID) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -730,6 +735,25 @@ public function validateSubmission(array $questions, array $answers, string $for | |||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Check one answer to a question answered on a numbered scale. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Shared by the linear scale and the rating, which differ only in how the scale is | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * drawn. The bounds and their defaults are the linear scale's; a rating does not | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * accept optionsLowest, so its lowest end always falls back to 1. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param array $question the question being answered | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param mixed $answer one submitted value | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @throws \InvalidArgumentException if the answer is not a whole number within range | ||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| private function validateScaleAnswer(array $question, mixed $answer): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $optionsLowest = $question['extraSettings']['optionsLowest'] ?? 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| $optionsHighest = $question['extraSettings']['optionsHighest'] ?? 5; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!ctype_digit((string)$answer) || intval($answer) < $optionsLowest || intval($answer) > $optionsHighest) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new \InvalidArgumentException(sprintf('The answer for question "%s" must be an integer between %d and %d.', $question['text'], $optionsLowest, $optionsHighest)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Validate correct date/time formats | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param array $answers Array with date from answer | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good in general, however I'm not sure about the presentation of the question. Google e.g. uses colored buttons (stars -> yellow, hearts -> red, thumbs -> blue). We could also use the whole width for displaying the icons like we do for linear scale questions. The settings could also be reworked a little and contain the icons and the texts. Google also has labels above the icons. But I think this is something our designers could review/judge best :) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,272 @@ | ||||||
| <!-- | ||||||
| - SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||||||
| - SPDX-License-Identifier: AGPL-3.0-or-later | ||||||
| --> | ||||||
|
|
||||||
| <template> | ||||||
| <Question | ||||||
| v-bind="questionProps" | ||||||
| :titlePlaceholder="answerType.titlePlaceholder" | ||||||
| :warningInvalid="answerType.warningInvalid" | ||||||
| :errorMessage="errorMessage" | ||||||
| v-on="commonListeners"> | ||||||
| <template #actions> | ||||||
| <NcActionInput | ||||||
| :modelValue="optionsHighest" | ||||||
| type="multiselect" | ||||||
| :clearable="false" | ||||||
| :label="t('forms', 'Number of icons')" | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we find a better wording here?
Suggested change
|
||||||
| labelOutside | ||||||
| :options="[2, 3, 4, 5, 6, 7, 8, 9, 10]" | ||||||
| required | ||||||
| @update:modelValue="onOptionsHighestChange"> | ||||||
| <template #icon> | ||||||
| <NcIconSvgWrapper :svg="outlineIcon" /> | ||||||
| </template> | ||||||
| </NcActionInput> | ||||||
| <NcActionRadio | ||||||
| v-for="icon in iconChoices" | ||||||
| :key="icon.id" | ||||||
| :modelValue="ratingIcon" | ||||||
| :name="`ratingIcon_${id}`" | ||||||
| :value="icon.id" | ||||||
| @update:modelValue="onRatingIconChange(icon.id)"> | ||||||
| {{ icon.label }} | ||||||
| </NcActionRadio> | ||||||
| </template> | ||||||
|
|
||||||
| <fieldset class="rating" :disabled="!readOnly"> | ||||||
| <legend class="hidden-visually"> | ||||||
| {{ text || t('forms', 'Rating') }} | ||||||
| </legend> | ||||||
| <label | ||||||
| v-for="value in optionsHighest" | ||||||
| :key="value" | ||||||
| class="rating__icon" | ||||||
| :class="{ 'rating__icon--on': value <= currentValue }"> | ||||||
| <input | ||||||
| class="hidden-visually" | ||||||
| type="radio" | ||||||
| :name="`rating_${id}`" | ||||||
| :aria-label=" | ||||||
| n('forms', '%n of {max}', '%n of {max}', value, { | ||||||
| max: optionsHighest, | ||||||
| }) | ||||||
|
Comment on lines
+52
to
+54
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why don't you use just |
||||||
| " | ||||||
| :value="value" | ||||||
| :checked="value === currentValue" | ||||||
| :required="isRequired && !currentValue" | ||||||
| @change="onPick(value)" /> | ||||||
| <NcIconSvgWrapper | ||||||
| :svg="value <= currentValue ? filledIcon : outlineIcon" /> | ||||||
| </label> | ||||||
| <NcButton | ||||||
| v-if="readOnly && currentValue" | ||||||
| variant="tertiary" | ||||||
| @click="onPick(0)"> | ||||||
| {{ t('forms', 'Clear') }} | ||||||
| </NcButton> | ||||||
|
Comment on lines
+63
to
+68
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need this button? Radio/Linear Scale question can't be cleared neither. If we really want to keep it, we should use a separate method |
||||||
| </fieldset> | ||||||
| </Question> | ||||||
| </template> | ||||||
|
|
||||||
| <script lang="ts"> | ||||||
| import IconHeartFilled from '@material-symbols/svg-400/outlined/favorite-fill.svg?raw' | ||||||
| import IconHeart from '@material-symbols/svg-400/outlined/favorite.svg?raw' | ||||||
| import IconStarFilled from '@material-symbols/svg-400/outlined/star-fill.svg?raw' | ||||||
| import IconStar from '@material-symbols/svg-400/outlined/star.svg?raw' | ||||||
| import IconThumbFilled from '@material-symbols/svg-400/outlined/thumb_up-fill.svg?raw' | ||||||
| import IconThumb from '@material-symbols/svg-400/outlined/thumb_up.svg?raw' | ||||||
| import { n, t } from '@nextcloud/l10n' | ||||||
| import { computed, defineComponent } from 'vue' | ||||||
| import NcActionInput from '@nextcloud/vue/components/NcActionInput' | ||||||
| import NcActionRadio from '@nextcloud/vue/components/NcActionRadio' | ||||||
| import NcButton from '@nextcloud/vue/components/NcButton' | ||||||
| import NcIconSvgWrapper from '@nextcloud/vue/components/NcIconSvgWrapper' | ||||||
| import Question from './Question.vue' | ||||||
| import { | ||||||
| QUESTION_EMITS, | ||||||
| QUESTION_PROPS, | ||||||
| useQuestion, | ||||||
| } from '../../composables/useQuestion.ts' | ||||||
|
|
||||||
| /** Matches the linear scale default the server assumes when optionsHighest is unset. */ | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could skip this comment here |
||||||
| const DEFAULT_OPTIONS_HIGHEST = 5 | ||||||
|
|
||||||
| export default defineComponent({ | ||||||
| name: 'QuestionRating', | ||||||
|
|
||||||
| components: { | ||||||
| NcActionInput, | ||||||
| NcActionRadio, | ||||||
| NcButton, | ||||||
| NcIconSvgWrapper, | ||||||
| Question, | ||||||
| }, | ||||||
|
|
||||||
| props: QUESTION_PROPS, | ||||||
| emits: [...QUESTION_EMITS, 'update:values'], | ||||||
|
|
||||||
| setup(props, { emit }) { | ||||||
| const question = useQuestion(props, { emit }) | ||||||
|
|
||||||
| const extraSettings = computed( | ||||||
| () => (props.extraSettings as Record<string, unknown> | undefined) ?? {}, | ||||||
| ) | ||||||
|
|
||||||
| const optionsHighest = computed<number>(() => { | ||||||
| const configured = extraSettings.value.optionsHighest | ||||||
| return typeof configured === 'number' | ||||||
| && configured >= 2 | ||||||
| && configured <= 10 | ||||||
| ? configured | ||||||
| : DEFAULT_OPTIONS_HIGHEST | ||||||
| }) | ||||||
|
|
||||||
| const ratingIcon = computed<string>(() => { | ||||||
| const icon = extraSettings.value.ratingIcon | ||||||
| return typeof icon === 'string' | ||||||
| && ['star', 'heart', 'thumb'].includes(icon) | ||||||
| ? icon | ||||||
| : 'star' | ||||||
| }) | ||||||
|
|
||||||
| const iconChoices = computed(() => [ | ||||||
| { id: 'star', label: t('forms', 'Stars') }, | ||||||
| { id: 'heart', label: t('forms', 'Hearts') }, | ||||||
| { id: 'thumb', label: t('forms', 'Thumbs up') }, | ||||||
| ]) | ||||||
|
|
||||||
| const outlineIcon = computed( | ||||||
| () => | ||||||
| ({ star: IconStar, heart: IconHeart, thumb: IconThumb })[ | ||||||
| ratingIcon.value | ||||||
| ], | ||||||
| ) | ||||||
|
|
||||||
| const filledIcon = computed( | ||||||
| () => | ||||||
| ({ | ||||||
| star: IconStarFilled, | ||||||
| heart: IconHeartFilled, | ||||||
| thumb: IconThumbFilled, | ||||||
| })[ratingIcon.value], | ||||||
| ) | ||||||
|
|
||||||
| const currentValue = computed<number>( | ||||||
| () => parseInt((props.values as string[])?.[0]) || 0, | ||||||
| ) | ||||||
|
|
||||||
| /** | ||||||
| * @param value the chosen count, or 0 to clear the answer | ||||||
| */ | ||||||
| function onPick(value: number): void { | ||||||
| emit('update:values', value ? [String(value)] : []) | ||||||
| question.errorMessage.value = null | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param value how many icons to offer | ||||||
| */ | ||||||
| function onOptionsHighestChange(value: number): void { | ||||||
| question.onExtraSettingsChange({ | ||||||
| optionsHighest: value === DEFAULT_OPTIONS_HIGHEST ? null : value, | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @param icon the chosen icon set | ||||||
| */ | ||||||
| function onRatingIconChange(icon: string): void { | ||||||
| question.onExtraSettingsChange({ | ||||||
| ratingIcon: icon === 'star' ? null : icon, | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A rating cannot be partly filled in, so the only failure is a required | ||||||
| * question left unanswered. | ||||||
| */ | ||||||
| async function validate(): Promise<boolean> { | ||||||
| if (props.isRequired && !currentValue.value) { | ||||||
| question.errorMessage.value = t( | ||||||
| 'forms', | ||||||
| 'You must answer this question', | ||||||
| ) | ||||||
| return false | ||||||
| } | ||||||
| question.errorMessage.value = null | ||||||
| return true | ||||||
| } | ||||||
|
|
||||||
| return { | ||||||
| ...question, | ||||||
| currentValue, | ||||||
| filledIcon, | ||||||
| iconChoices, | ||||||
| optionsHighest, | ||||||
| n, | ||||||
| onOptionsHighestChange, | ||||||
| onPick, | ||||||
| onRatingIconChange, | ||||||
| outlineIcon, | ||||||
| ratingIcon, | ||||||
| t, | ||||||
| validate, | ||||||
| } | ||||||
| }, | ||||||
| }) | ||||||
| </script> | ||||||
|
|
||||||
| <style lang="scss" scoped> | ||||||
| .rating { | ||||||
| align-items: center; | ||||||
| border: none; | ||||||
| display: flex; | ||||||
| // Ten icons plus a Clear button do not fit one line on a narrow screen. | ||||||
| flex-wrap: wrap; | ||||||
| gap: 2px; | ||||||
| margin: 0; | ||||||
| padding: 0; | ||||||
|
|
||||||
| &__icon { | ||||||
| align-items: center; | ||||||
| border-radius: var(--border-radius); | ||||||
| color: var(--color-text-maxcontrast); | ||||||
| cursor: pointer; | ||||||
| display: inline-flex; | ||||||
| justify-content: center; | ||||||
| // A comfortable pointer target; the icon itself stays small. | ||||||
| min-height: var(--default-clickable-area); | ||||||
| min-width: var(--default-clickable-area); | ||||||
| transition: | ||||||
| color 0.1s ease-in-out, | ||||||
| transform 0.1s ease-in-out; | ||||||
|
|
||||||
| &--on { | ||||||
| color: var(--color-favorite, var(--color-warning)); | ||||||
| } | ||||||
|
|
||||||
| &:hover { | ||||||
| transform: scale(1.1); | ||||||
| } | ||||||
|
|
||||||
| &:focus-within { | ||||||
| outline: 2px solid var(--color-primary-element); | ||||||
| outline-offset: -2px; | ||||||
| } | ||||||
|
|
||||||
| @media (prefers-reduced-motion: reduce) { | ||||||
| transition: none; | ||||||
|
|
||||||
| &:hover { | ||||||
| transform: none; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| &:disabled &__icon { | ||||||
| cursor: default; | ||||||
| } | ||||||
| } | ||||||
| </style> | ||||||
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.