-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
feat: min max chars for long text #15592
feat: min max chars for long text #15592
Conversation
@mhetreayush is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (06/27/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (06/27/24)1 label was added to this PR based on Keith Williams's automation. |
ae7b296
to
044a324
Compare
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.
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.
@calcom/platform this looks fine for me but would like your eyes before approvals/merging as i'm not 100% sure if you are dating these upon every change
@sean-brydon thank you for a quick reply. |
minLength: z.number().optional(), | ||
maxLength: z.number().optional(), |
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.
@calcom/platform Is it possible to reuse packages/features/form-builder/schema.ts
somehow 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.
@mhetreayush Thanks for the PR 🙏
Left a few suggestions
@sean-brydon @hariombalhara Also took care of the reviews |
apps/api/v2/src/ee/event-types/event-types_2024_04_15/inputs/update-event-type.input.ts
Show resolved
Hide resolved
apps/api/v2/src/ee/event-types/event-types_2024_04_15/outputs/get-event-type-public.output.ts
Show resolved
Hide resolved
faf765d
to
e5c68ed
Compare
9fb138a
to
444a44f
Compare
@@ -0,0 +1 @@ | |||
export const HARD_LIMIT_MAX_LENGTH = 4000; |
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 a limit for textarea only. How about we keep it in form-builder/fieldTypes as textarea.hardLimitOnMaxLength.
In this way it is better organized.
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 am taking care of it. I will play around and see what is best, as it's been a long time I worked on form builder
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.
Fair enough, can do.
…chars' into features/long-text-min-max-chars
supportsLengthCheck: { | ||
maxLength: 4000, | ||
}, |
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.
@mhetreayush Added maxLength support here. It easily allows any field that wants to support length check feature to define maxLength. Infact it is mandatory to define maxLength if someone enables length check support.
This should also solve the problem of easily configuring it for text field type.
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 different from the maxLength
that we have in baseFieldSchema
as that can be changed by the user per field. This on the other hand is constant and defined by our code and applies to all fields of that type
@@ -0,0 +1,158 @@ | |||
import { expect } from "@playwright/test"; |
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.
@mhetreayush We have playwright/manage-booking-questions.e2e.ts for form builder related tests.
We should move this test there.
Also, could you see if we can test textarea
field along with any existing test in there? If not that's also fine. The tests there follow the same flow, it is just about adding and verifying a new field.
I wanted to ensure that we don't introduce new e2es when when we can amend any existing e2e. As an e2e test is time consuming.
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.
Will do that.
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.
Also, maybe we could also add unit test for textarea
schema in schema.ts.
And, supportsLengthCheck related component code can also be unit tested with react-testing-library. For that you would have to refactor supportsLengthCheck related code into a new component(which really makes sense anyway)
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 Forty-two Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored. |
@@ -105,7 +105,8 @@ const configMap: Record<FieldType, Omit<z.infer<typeof fieldTypeConfigSchema>, " | |||
value: "textarea", | |||
isTextType: true, | |||
supportsLengthCheck: { | |||
maxLength: 4000, | |||
// Keep it as small as possible. It is easier to change to a higher value but coming back to a lower value(due to any reason) would be problematic for users who have saved higher value. | |||
maxLength: 1000, |
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.
@hariombalhara Can we use a shared constant here instead of a direct value? Bringing this up since you mentioned it is constant to our code base.
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 can define a local HARD_MAX_LENGTH_TEXTAREA locally in this module.
It isn't a constant to be used throughout the app now.
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 can define a local HARD_MAX_LENGTH_TEXTAREA locally in this module.
Yes this would be better I believe
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'll take care of that @hariombalhara
@hariombalhara I am actually unavailable for some time, so will be inactive on the issue, could you or someone else take this ahead? |
@mhetreayush I would like to collaborate with you and make progress on this issue. I will take on this issue :) |
Thanks @varshith257 , I will cancel my claim attempt then |
@mhetreayush |
Closing it in favour of #15693 which improves over the changes in this PR. |
/claim #15577
What does this PR do?
This PR adds an optional minimum and maximum character limit to long text question in booking form.
Loom Video: https://www.loom.com/share/735a6768a92847cabefd1fe2187c499d?sid=2d606562-c6e6-4bd3-92c0-b1fcdab14218
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Are there environment variables that should be set?
No
What are the minimal test data to have?
N.A.
What is expected (happy path) to have (input and output)?
Event Types
in the sidebar.Advanced
tab on the left sidebar.Long text
Preview
button in the event header.a. Min &/or max character limit unsatisfied -> should show red text with error.
b. All the conditions satisfied -> Should save successfully.
Checklist