-
Notifications
You must be signed in to change notification settings - Fork 617
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
Rules for Condition Groups #3737
base: next
Are you sure you want to change the base?
Conversation
…s for condition groups
On this one, I've got two main points of feedback:
|
packages/app-form-builder/src/components/Form/functions/getNextStepIndex.ts
Outdated
Show resolved
Hide resolved
packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/Styled.ts
Outdated
Show resolved
Hide resolved
packages/app-page-builder-elements/src/renderers/form/FormRender/functions/getNextStepIndex.ts
Show resolved
Hide resolved
...form-builder/src/admin/components/FormEditor/Context/functions/handleDeleteConditionGroup.ts
Outdated
Show resolved
Hide resolved
packages/app-form-builder/src/admin/components/FormEditor/Context/functions/handleMoveRow.ts
Outdated
Show resolved
Hide resolved
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.
Left some comments.
And while I think we can tackle it, I think it's also important we check @SvenAlHamad's comments he posted some time ago.
Can we check that out? I'd guess tackling that feedback will introduce more changes into the PR.
packages/app-form-builder/src/admin/components/FormEditor/Tabs/EditTab/Styled.ts
Outdated
Show resolved
Hide resolved
packages/app-form-builder/src/components/Form/functions/getNextStepIndex.ts
Outdated
Show resolved
Hide resolved
packages/app-page-builder-elements/src/renderers/form/FormRender/functions/getNextStepIndex.ts
Show resolved
Hide resolved
packages/app-page-builder-elements/src/renderers/form/FormRender/functions/getNextStepIndex.ts
Show resolved
Hide resolved
I think that we can introduce requested changes in this PR. |
Hey @neatbyte-ivelychko @neatbyte-vnobis , I was just checking the I went to bundlephobia and entered As you can see in the sshot, it's 28.4kb, which is not small. But, the more interesting part is that the lib actually requires All in all... I know we're going back and forth on this, but.... Since here we're talking about like.... what.... 6 validators? Can we just create a Again, if this was an admin facing pkg, we wouldn't care. But we gotta care when it comes to code that's about to get served on public website. I believe that if we coded it manually like explained above, we'd probably end up with less then 1kb of extra code. Wdyt? |
Yeah, will do that as soon as I finish working on |
Should we do the same steps for |
Yes please. Maybe it's worth exploring Let's see how much code there will be and if there's a solid amount of code to c/p, then having a shared package might be a better idea. |
Btw. Maybe we should consider adding unit tests for validation as we have done it in IconPicker? |
@adrians5j what do you think about this? |
@SvenAlHamad 1 - Rules for step |
fc7c162
to
b7e4ba4
Compare
Changes
Added rules for condition groups.
How Has This Been Tested?
Manually.
Documentation
None.