From a363b1e903c472fe22ad91daf5807db7254ef7e5 Mon Sep 17 00:00:00 2001 From: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com> Date: Thu, 6 Apr 2023 13:47:53 +0530 Subject: [PATCH] fix: ui and ux improvements in booking questions (#7734) --- .../components/booking/pages/BookingPage.tsx | 24 +--- .../components/eventtype/EventAdvancedTab.tsx | 6 + .../manage-booking-questions.e2e.ts | 10 +- apps/web/public/static/locales/en/common.json | 3 +- apps/web/public/static/locales/fr/common.json | 2 - apps/web/public/static/locales/he/common.json | 2 - apps/web/public/static/locales/ru/common.json | 2 - .../features/bookings/lib/getBookingFields.ts | 4 +- .../bookings/lib/getBookingResponsesSchema.ts | 11 +- .../lib/getLocationOptionsForSelect.ts | 29 ++++ packages/features/form-builder/Components.tsx | 15 +- .../features/form-builder/FormBuilder.tsx | 130 +++++++++++------- .../form-builder/FormBuilderFieldsSchema.ts | 11 ++ packages/ui/components/badge/Badge.tsx | 1 + packages/ui/components/form/switch/Switch.tsx | 26 ++-- .../form/toggleGroup/BooleanToggleGroup.tsx | 7 +- 16 files changed, 183 insertions(+), 100 deletions(-) create mode 100644 packages/features/bookings/lib/getLocationOptionsForSelect.ts diff --git a/apps/web/components/booking/pages/BookingPage.tsx b/apps/web/components/booking/pages/BookingPage.tsx index e68bf27033..9620f2d65c 100644 --- a/apps/web/components/booking/pages/BookingPage.tsx +++ b/apps/web/components/booking/pages/BookingPage.tsx @@ -11,7 +11,6 @@ import { z } from "zod"; import BookingPageTagManager from "@calcom/app-store/BookingPageTagManager"; import type { EventLocationType } from "@calcom/app-store/locations"; -import { getEventLocationType, locationKeyToString } from "@calcom/app-store/locations"; import { createPaymentLink } from "@calcom/app-store/stripepayment/lib/client"; import { getEventTypeAppData } from "@calcom/app-store/utils"; import type { LocationObject } from "@calcom/core/location"; @@ -29,6 +28,7 @@ import { import getBookingResponsesSchema, { getBookingResponsesPartialSchema, } from "@calcom/features/bookings/lib/getBookingResponsesSchema"; +import getLocationOptionsForSelect from "@calcom/features/bookings/lib/getLocationOptionsForSelect"; import { FormBuilderField } from "@calcom/features/form-builder/FormBuilder"; import classNames from "@calcom/lib/classNames"; import { APP_NAME } from "@calcom/lib/constants"; @@ -143,26 +143,14 @@ const BookingFields = ({ } const optionsInputs = field.optionsInputs; - const options = locations.map((location) => { - const eventLocation = getEventLocationType(location.type); - const locationString = locationKeyToString(location); - - if (typeof locationString !== "string" || !eventLocation) { - // It's possible that location app got uninstalled - return null; - } - const type = eventLocation.type; - const optionInput = optionsInputs[type as keyof typeof optionsInputs]; + // TODO: Instead of `getLocationOptionsForSelect` options should be retrieved from dataStore[field.getOptionsAt]. It would make it agnostic of the `name` of the field. + const options = getLocationOptionsForSelect(locations, t); + options.forEach((option) => { + const optionInput = optionsInputs[option.value as keyof typeof optionsInputs]; if (optionInput) { - optionInput.placeholder = t(eventLocation?.attendeeInputPlaceholder || ""); + optionInput.placeholder = option.inputPlaceholder; } - - return { - label: t(locationString), - value: type, - }; }); - field.options = options.filter( (location): location is NonNullable<(typeof options)[number]> => !!location ); diff --git a/apps/web/components/eventtype/EventAdvancedTab.tsx b/apps/web/components/eventtype/EventAdvancedTab.tsx index 4e7a57a6ad..f9ec5cafe8 100644 --- a/apps/web/components/eventtype/EventAdvancedTab.tsx +++ b/apps/web/components/eventtype/EventAdvancedTab.tsx @@ -8,6 +8,7 @@ import { v5 as uuidv5 } from "uuid"; import type { EventNameObjectType } from "@calcom/core/event"; import { getEventName } from "@calcom/core/event"; +import getLocationsOptionsForSelect from "@calcom/features/bookings/lib/getLocationOptionsForSelect"; import DestinationCalendarSelector from "@calcom/features/calendars/DestinationCalendarSelector"; import { FormBuilder } from "@calcom/features/form-builder/FormBuilder"; import { classNames } from "@calcom/lib"; @@ -148,6 +149,11 @@ export const EventAdvancedTab = ({ eventType, team }: Pick
{ test.afterEach(async ({ users }) => { await users.deleteAll(); @@ -91,7 +95,7 @@ async function runTestStepsCommonForTeamAndUserEventType( await expect(userFieldLocator.locator('[name="how_are_you"]')).toBeVisible(); // There are 2 labels right now. Will be one in future. The second one is hidden - expect(await userFieldLocator.locator("label").nth(0).innerText()).toBe("How are you?"); + expect(await getLabelText(userFieldLocator)).toBe("How are you?"); await expect(userFieldLocator.locator("input[type=text]")).toBeVisible(); }); }); @@ -140,7 +144,7 @@ async function runTestStepsCommonForTeamAndUserEventType( expect( await formBuilderFieldLocator.locator('[name="how_are_you"]').getAttribute("placeholder") ).toBe("I'm fine, thanks"); - expect(await formBuilderFieldLocator.locator("label").nth(0).innerText()).toBe("How are you?"); + expect(await getLabelText(formBuilderFieldLocator)).toBe("How are you?"); await formBuilderFieldLocator.locator('[name="how_are_you"]').fill("I am great!"); await bookTimeSlot({ page, name: "Booker", email: "booker@example.com" }); await expect(page.locator("[data-testid=success-page]")).toBeVisible(); diff --git a/apps/web/public/static/locales/en/common.json b/apps/web/public/static/locales/en/common.json index 148c0d076e..60a6205336 100644 --- a/apps/web/public/static/locales/en/common.json +++ b/apps/web/public/static/locales/en/common.json @@ -1625,8 +1625,7 @@ "not_enough_seats": "Not enough seats", "form_builder_field_already_exists": "A field with this name already exists", "form_builder_field_add_subtitle": "Customize the questions asked on the booking page", - "form_builder_system_field_cant_delete": "This system field can't be removed.", - "form_builder_system_field_cant_toggle": "This system field can't be toggled.", + "show_on_booking_page":"Show on booking page", "get_started_zapier_templates": "Get started with Zapier templates", "team_is_unpublished": "{{team}} is unpublished", "team_is_unpublished_description": "This team link is currently not available. Please contact the team owner or ask them publish it.", diff --git a/apps/web/public/static/locales/fr/common.json b/apps/web/public/static/locales/fr/common.json index 1013709a16..81b09891ff 100644 --- a/apps/web/public/static/locales/fr/common.json +++ b/apps/web/public/static/locales/fr/common.json @@ -1614,8 +1614,6 @@ "booking_confirmation_failed": "Échec de la confirmation de la réservation", "form_builder_field_already_exists": "Un champ avec ce nom existe déjà", "form_builder_field_add_subtitle": "Personnalisez les questions posées sur la page de réservation.", - "form_builder_system_field_cant_delete": "Ce champ système ne peut pas être supprimé.", - "form_builder_system_field_cant_toggle": "Ce champ système ne peut pas être activé.", "get_started_zapier_templates": "Démarrer avec les modèles Zapier", "team_member": "Membre d'équipe", "a_routing_form": "Un formulaire de routage", diff --git a/apps/web/public/static/locales/he/common.json b/apps/web/public/static/locales/he/common.json index 800c637a87..98de66cbe8 100644 --- a/apps/web/public/static/locales/he/common.json +++ b/apps/web/public/static/locales/he/common.json @@ -1614,8 +1614,6 @@ "booking_confirmation_failed": "אישור ההזמנה נכשל", "form_builder_field_already_exists": "שדה עם שם זה כבר קיים", "form_builder_field_add_subtitle": "התאמת השאלות שנשאלות בדף התזמון", - "form_builder_system_field_cant_delete": "שדה מערכת זה לא ניתן להסרה.", - "form_builder_system_field_cant_toggle": "שדה מערכת זה לא ניתן לשינוי.", "get_started_zapier_templates": "התחל עם תבניות Zapier", "team_is_unpublished": "צוות {{team}} אינו מפורסם", "team_is_unpublished_description": "קישור הצוות הזה אינו זמין כעת. אנא צור קשר עם הבעלים של הצוות או בקש מהם לפרסם אותו.", diff --git a/apps/web/public/static/locales/ru/common.json b/apps/web/public/static/locales/ru/common.json index edba49a9fe..a9882211e9 100644 --- a/apps/web/public/static/locales/ru/common.json +++ b/apps/web/public/static/locales/ru/common.json @@ -1614,8 +1614,6 @@ "booking_confirmation_failed": "Не удалось подтвердить бронирование", "form_builder_field_already_exists": "Поле с таким именем уже существует", "form_builder_field_add_subtitle": "Настроить вопросы, задаваемые на странице бронирования", - "form_builder_system_field_cant_delete": "Это системное поле нельзя удалить.", - "form_builder_system_field_cant_toggle": "Это системное поле нельзя переключить.", "get_started_zapier_templates": "Начать работу с шаблонами Zapier", "team_member": "Участник команды", "a_routing_form": "Форма маршрутизации", diff --git a/packages/features/bookings/lib/getBookingFields.ts b/packages/features/bookings/lib/getBookingFields.ts index 932807c9b8..5594185c9c 100644 --- a/packages/features/bookings/lib/getBookingFields.ts +++ b/packages/features/bookings/lib/getBookingFields.ts @@ -191,9 +191,9 @@ export const ensureBookingInputsHaveSystemFields = ({ type: "radioInput", name: "location", editable: "system", + hideWhenJustOneOption: true, required: false, - // Populated on the fly from locations. I don't want to duplicate storing locations and instead would like to be able to refer to locations in eventType. - // options: `eventType.locations` + getOptionsAt: "locations", optionsInputs: { attendeeInPerson: { type: "address", diff --git a/packages/features/bookings/lib/getBookingResponsesSchema.ts b/packages/features/bookings/lib/getBookingResponsesSchema.ts index 95789243ca..a4a68ecd51 100644 --- a/packages/features/bookings/lib/getBookingResponsesSchema.ts +++ b/packages/features/bookings/lib/getBookingResponsesSchema.ts @@ -121,12 +121,13 @@ function preprocess({ const views = bookingField.views; const isFieldApplicableToCurrentView = currentView === "ALL_VIEWS" ? true : views ? views.find((view) => view.id === currentView) : true; + let hidden = bookingField.hidden; + const numOptions = bookingField.options?.length ?? 0; + if (bookingField.hideWhenJustOneOption) { + hidden = hidden || numOptions <= 1; + } // If the field is hidden, then it can never be required - const isRequired = bookingField.hidden - ? false - : isFieldApplicableToCurrentView - ? bookingField.required - : false; + const isRequired = hidden ? false : isFieldApplicableToCurrentView ? bookingField.required : false; if ((isPartialSchema || !isRequired) && value === undefined) { return; diff --git a/packages/features/bookings/lib/getLocationOptionsForSelect.ts b/packages/features/bookings/lib/getLocationOptionsForSelect.ts new file mode 100644 index 0000000000..1b1ea7e2f4 --- /dev/null +++ b/packages/features/bookings/lib/getLocationOptionsForSelect.ts @@ -0,0 +1,29 @@ +import type { LocationObject } from "@calcom/app-store/locations"; +import { locationKeyToString } from "@calcom/app-store/locations"; +import { getEventLocationType } from "@calcom/app-store/locations"; +import type { useLocale } from "@calcom/lib/hooks/useLocale"; +import notEmpty from "@calcom/lib/notEmpty"; + +export default function getLocationsOptionsForSelect( + locations: LocationObject[], + t: ReturnType["t"] +) { + return locations + .map((location) => { + const eventLocation = getEventLocationType(location.type); + const locationString = locationKeyToString(location); + + if (typeof locationString !== "string" || !eventLocation) { + // It's possible that location app got uninstalled + return null; + } + const type = eventLocation.type; + + return { + label: t(locationString), + value: type, + inputPlaceholder: t(eventLocation?.attendeeInputPlaceholder || ""), + }; + }) + .filter(notEmpty); +} diff --git a/packages/features/form-builder/Components.tsx b/packages/features/form-builder/Components.tsx index dc8e8e5fb9..ca9d58f98e 100644 --- a/packages/features/form-builder/Components.tsx +++ b/packages/features/form-builder/Components.tsx @@ -48,6 +48,7 @@ type Component = value: { value: string; optionValue: string }; } & { name?: string; + required?: boolean; } >( props: TProps @@ -323,8 +324,14 @@ export const Components: Record = { }) ) : ( // Show option itself as label because there is just one option - // TODO: Support asterisk for required fields - + <> + + )} @@ -342,10 +349,10 @@ export const Components: Record = { name: "optionField", }} value={value?.optionValue} - setValue={(val: string) => { + setValue={(val: string | undefined) => { setValue({ value: value?.value, - optionValue: val, + optionValue: val || "", }); }} /> diff --git a/packages/features/form-builder/FormBuilder.tsx b/packages/features/form-builder/FormBuilder.tsx index fe1b573b41..01761df137 100644 --- a/packages/features/form-builder/FormBuilder.tsx +++ b/packages/features/form-builder/FormBuilder.tsx @@ -44,11 +44,18 @@ export const FormBuilder = function FormBuilder({ description, addFieldLabel, formProp, + dataStore, }: { formProp: string; title: string; description: string; addFieldLabel: string; + /** + * A readonly dataStore that is used to lookup the options for the fields. It works in conjunction with the field.getOptionAt property which acts as the key in options + */ + dataStore: { + options: Record; + }; }) { const FieldTypesMap: Record< string, @@ -112,6 +119,8 @@ export const FormBuilder = function FormBuilder({ value: "radioInput", isTextType: false, systemOnly: true, + // This is false currently because we don't want to show the options for Location field right now. It is the only field with type radioInput. + // needsOptions: true, }, checkbox: { label: "Checkbox Group", @@ -148,13 +157,14 @@ export const FormBuilder = function FormBuilder({ function OptionsField({ label = "Options", value, - onChange, + // eslint-disable-next-line @typescript-eslint/no-empty-function + onChange = () => {}, className = "", readOnly = false, }: { label?: string; value: { label: string; value: string }[]; - onChange: (value: { label: string; value: string }[]) => void; + onChange?: (value: { label: string; value: string }[]) => void; className?: string; readOnly?: boolean; }) { @@ -257,6 +267,7 @@ export const FormBuilder = function FormBuilder({ const fieldType = FieldTypesMap[fieldForm.watch("type") || "text"]; const isFieldEditMode = fieldDialog.fieldIndex !== -1; + return (
@@ -264,9 +275,20 @@ export const FormBuilder = function FormBuilder({

{description}

    {fields.map((field, index) => { + const options = field.options + ? field.options + : field.getOptionsAt + ? dataStore.options[field.getOptionsAt as keyof typeof dataStore] + : []; + const numOptions = options?.length ?? 0; + if (field.hideWhenJustOneOption && numOptions <= 1) { + return null; + } const fieldType = FieldTypesMap[field.type]; - const isRequired = field.required; + const isFieldEditableSystemButOptional = field.editable === "system-but-optional"; + const isFieldEditableSystem = field.editable === "system"; + const isUserField = !isFieldEditableSystem && !isFieldEditableSystemButOptional; if (!fieldType) { throw new Error(`Invalid field type - ${field.type}`); @@ -286,7 +308,7 @@ export const FormBuilder = function FormBuilder({
  • + className="hover:bg-muted group relative flex items-center justify-between p-4 "> {index >= 1 && ( -
)} @@ -434,7 +456,7 @@ export const FormBuilder = function FormBuilder({ }} value={FieldTypesMap[fieldForm.getValues("type")]} options={FieldTypes.filter((f) => !f.systemOnly)} - label="Input Type" + label={t("input_type")} /> ) : null} - - {fieldType?.needsOptions ? ( + {fieldType?.needsOptions && !fieldForm.getValues("getOptionsAt") ? ( { @@ -471,6 +492,14 @@ export const FormBuilder = function FormBuilder({ }} /> ) : null} + {/* TODO: Maybe we should show location options in readOnly mode in Booking Questions. Right now options are not shown in Manage Booking Questions UI for location Booking Question */} + {/* {fieldForm.getValues("getOptionsAt") ? ( + + ) : null} */} - Cancel + {t("cancel")} @@ -520,10 +549,12 @@ const WithLabel = ({ {/* Component itself managing it's label should remove these checks */} {field.type !== "boolean" && field.type !== "multiemail" && field.label && (
- - - {!readOnly && field.required ? "*" : ""} - +
)} {children} @@ -587,7 +618,6 @@ export const ComponentForField = ({ `Value ${value} is not valid for type ${componentConfig.propsType} for field ${field.name}` ); } - if (componentConfig.propsType === "text") { return ( @@ -683,6 +713,7 @@ export const ComponentForField = ({ setValue={setValue as (arg: typeof value) => void} optionsInputs={field.optionsInputs} options={field.options} + required={field.required} /> ) : null; @@ -708,7 +739,7 @@ export const FormBuilderField = ({ control={control} // Make it a variable name={`responses.${field.name}`} - render={({ field: { value, onChange } }) => { + render={({ field: { value, onChange }, fieldState: { error } }) => { return (
{ - const name = message?.replace(/\{([^}]+)\}.*/, "$1"); - // Use the message targeted for it. - if (name !== field.name) { + render={({ message }: { message: string | undefined }) => { + message = message || ""; + // If the error comes due to parsing the `responses` object(which can have error for any field), we need to identify the field that has the error from the message + const name = message.replace(/\{([^}]+)\}.*/, "$1"); + const isResponsesErrorForThisField = name === field.name; + // If the error comes for the specific property of responses(Possible for system fields), then also we would go ahead and show the error + if (!isResponsesErrorForThisField && !error) { return null; } @@ -738,7 +772,7 @@ export const FormBuilderField = ({ data-testid={`error-message-${field.name}`} className="mt-2 flex items-center text-sm text-red-700 "> -

{t(message)}

+

{t(message || "invalid_input")}

); }} diff --git a/packages/features/form-builder/FormBuilderFieldsSchema.ts b/packages/features/form-builder/FormBuilderFieldsSchema.ts index 9dbc2840cb..caecfda232 100644 --- a/packages/features/form-builder/FormBuilderFieldsSchema.ts +++ b/packages/features/form-builder/FormBuilderFieldsSchema.ts @@ -44,7 +44,14 @@ const fieldSchema = z.object({ .array() .optional(), type: fieldTypeEnum, + options: z.array(z.object({ label: z.string(), value: z.string() })).optional(), + /** + * This is an alternate way to specify options when the options are stored elsewhere. Form Builder expects options to be present at `dataStore[getOptionsAt]` + * This allows keeping a single source of truth in DB. + */ + getOptionsAt: z.string().optional(), + optionsInputs: z .record( z.object({ @@ -56,6 +63,10 @@ const fieldSchema = z.object({ }) ) .optional(), + /** + * It is used to hide fields such as location when there are less than two options + */ + hideWhenJustOneOption: z.boolean().default(false).optional(), required: z.boolean().default(false).optional(), hidden: z.boolean().optional(), diff --git a/packages/ui/components/badge/Badge.tsx b/packages/ui/components/badge/Badge.tsx index 88531b99a1..cdf2b89adb 100644 --- a/packages/ui/components/badge/Badge.tsx +++ b/packages/ui/components/badge/Badge.tsx @@ -18,6 +18,7 @@ const badgeStyles = cva("font-medium inline-flex items-center justify-center rou blue: "bg-info text-info", red: "bg-error text-error", error: "bg-error text-error", + grayWithoutHover: "bg-gray-100 text-gray-800 dark:bg-darkgray-200 dark:text-darkgray-800", }, size: { sm: "px-1 py-0.5 text-xs leading-3", diff --git a/packages/ui/components/form/switch/Switch.tsx b/packages/ui/components/form/switch/Switch.tsx index 055c24a348..1b90cf4c8d 100644 --- a/packages/ui/components/form/switch/Switch.tsx +++ b/packages/ui/components/form/switch/Switch.tsx @@ -3,7 +3,7 @@ import * as Label from "@radix-ui/react-label"; import * as PrimitiveSwitch from "@radix-ui/react-switch"; import React from "react"; -import classNames from "@calcom/lib/classNames"; +import cx from "@calcom/lib/classNames"; import { Tooltip } from "../../tooltip"; @@ -16,21 +16,27 @@ const Wrapper = ({ children, tooltip }: { tooltip?: string; children: React.Reac const Switch = ( props: React.ComponentProps & { label?: string; - thumbProps?: { - className?: string; - }; fitToHeight?: boolean; tooltip?: string; + classNames?: { + container?: string; + thumb?: string; + }; } ) => { - const { label, fitToHeight, ...primitiveProps } = props; + const { label, fitToHeight, classNames, ...primitiveProps } = props; const id = useId(); return ( -
+
{label && ( diff --git a/packages/ui/components/form/toggleGroup/BooleanToggleGroup.tsx b/packages/ui/components/form/toggleGroup/BooleanToggleGroup.tsx index 8bdb57d9b7..588286bdbc 100644 --- a/packages/ui/components/form/toggleGroup/BooleanToggleGroup.tsx +++ b/packages/ui/components/form/toggleGroup/BooleanToggleGroup.tsx @@ -32,8 +32,10 @@ export const BooleanToggleGroup = function BooleanToggleGroup({ onValueChange(defaultValue); return null; } - const commonClass = - "w-full inline-flex items-center justify-center rounded py-[10px] px-4 text-sm font-medium leading-4"; + const commonClass = classNames( + "w-full inline-flex items-center justify-center rounded py-[10px] px-4 text-sm font-medium leading-4", + disabled && "cursor-not-allowed" + ); const selectedClass = classNames(commonClass, "bg-emphasis text-emphasis"); const unselectedClass = classNames(commonClass, "text-default hover:bg-subtle hover:text-emphasis"); return ( @@ -53,6 +55,7 @@ export const BooleanToggleGroup = function BooleanToggleGroup({ value="yes"> Yes +