Skip to content
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

Open
wants to merge 11 commits into
base: next
Choose a base branch
from

Conversation

neatbyte-vnobis
Copy link
Collaborator

Changes

Added rules for condition groups.

How Has This Been Tested?

Manually.

Documentation

None.

@neatbyte-vnobis neatbyte-vnobis marked this pull request as ready for review December 1, 2023 15:41
@SvenAlHamad
Copy link
Contributor

On this one, I've got two main points of feedback:

  1. UI: We've recently delivered a new feature called Advanced Search. I would like to use the look and feel of that accordion (styling, bg color) and the arrangement of the fields and conditions as well as the match all/any option. I think it will make the UI more uniformed across the apps.

  2. Submit & redirect: While talking to one of our customers about the conditional steps they brought forward a project they are currently working on where they need to send users to different pages depending on which step the user submitted. At the moment under Rules we have a dropdown with options: "Got to step" and "Submit", I would like to add here a 3rd option: Submit & Redirect. This would display an input where the user can define the url -> It should work the same way as the "Redirect" trigger that we have. The submit & redirect will take priority over the generic redirect trigger.

@adrians5j adrians5j self-assigned this Dec 14, 2023
Copy link
Member

@adrians5j adrians5j left a 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.

@neatbyte-ivelychko
Copy link

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.

I think that we can introduce requested changes in this PR.

@adrians5j
Copy link
Member

adrians5j commented Jan 4, 2024

Hey @neatbyte-ivelychko @neatbyte-vnobis , I was just checking the lodash / @webiny/validation changes. Looks good... but...... I did some more thinking here.

I went to bundlephobia and entered @webiny/validation to see what's the size of @webiny/validation.

As you can see in the sshot, it's 28.4kb, which is not small.

image

But, the more interesting part is that the lib actually requires lodash internally 😅😅😅

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 validators folder in page-builder-elements package and literally have validators manually coded in there? Without any libs?

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?

@neatbyte-ivelychko
Copy link

neatbyte-ivelychko commented Jan 4, 2024

But, the more interesting part is that the lib actually requires lodash internally 😅😅😅

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 validators folder in page-builder-elements package and literally have validators manually coded in there? Without any libs?

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.

Yeah, will do that as soon as I finish working on Submit & Redirect rule action

@neatbyte-ivelychko
Copy link

But, the more interesting part is that the lib actually requires lodash internally 😅😅😅
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 validators folder in page-builder-elements package and literally have validators manually coded in there? Without any libs?
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.

Yeah, will do that as soon as I finish working on Submit & Redirect rule action

Should we do the same steps for app-form-builder or should we leave @webiny/validation there?

@adrians5j
Copy link
Member

Yes please.

Maybe it's worth exploring @webiny/app-form-builder-condition-groups-validation shared package? 🤔

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.

@adrians5j adrians5j added this to the 5.40.0 milestone Jan 4, 2024
@neatbyte-ivelychko
Copy link

Yes please.

Maybe it's worth exploring @webiny/app-form-builder-condition-groups-validation shared package? 🤔

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?
Because we would not need to manually test validation methods.

@neatbyte-ivelychko
Copy link

Btw. Maybe we should consider adding unit tests for validation as we have done it in IconPicker? Because we would not need to manually test validation methods.

@adrians5j what do you think about this?

@neatbyte-ivelychko
Copy link

@SvenAlHamad
Hi!
I have updated UI for Rules tabs.
Looking forward to your feedback!

1 - Rules for step
Знімок екрана 2024-01-11 о 14 20 30
2 - Rules for condition group
Знімок екрана 2024-01-11 о 14 22 30
3 - Rule with Submit & Redirect action
Знімок екрана 2024-01-11 о 14 24 58

@neatbyte-vnobis neatbyte-vnobis force-pushed the neatbyte/condition-group-field-rules branch from fc7c162 to b7e4ba4 Compare January 12, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants