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

feat(pieces-form): add separator, title, group #3206

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Doskyft
Copy link
Contributor

@Doskyft Doskyft commented Nov 16, 2023

Hello !

I am currently working on the creation of a piece which contains A LOT of fields, to improve the experience, I added "PropertyStyle", to be able to separate and group the fields together.

A small breaking change, mandatory fields are no longer put first.

Example
pieces-form-group
pieces-form-title

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2023

CLA assistant check
All committers have signed the CLA.

Copy link

nx-cloud bot commented Nov 16, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6f6f1d2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


🟥 Failed Commands
nx affected --target=lint --parallel=3
✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@abuaboud
Copy link
Contributor

Nice, @Doskyft!

The first picture seems like a group, and the second one looks like a title with a separator—kind of like creating a form group on the frontend, which makes sense.

I'm curious, though, about why someone would prefer Title + Separator over a Group. Any specific reasons or examples you have in mind? What's your take on this?

One drawback of having both is that it might confuse contributors about which props to use. Eventually, we could end up with a mix of opinions, leading to confusion or inconsistency from the end user's perspective.

In my view, the frontend for activepieces should have a clear opinion on how things are displayed, making it easier to apply new learnings across all pieces. However, if the functionality differs, we should support the prop.

In a group, I see it as more functional, eliminating the need for contributors to spread the object over many properties and assemble them in the code, as in the example you mentioned.

@abuaboud abuaboud added the 🤷‍♂️ action required Further action or information required label Nov 17, 2023
@Doskyft
Copy link
Contributor Author

Doskyft commented Nov 17, 2023

Hello @abuaboud,

I must admit that I didn't delve deeply into this matter. In the logic I had in mind, for instance, mandatory fields could always be displayed, but if there are many, they could be separated by separators or titles, while groups could be used for more optional fields. Additionally, it's worth noting that separators can also be utilized within groups for further organization.

However, while creating these different options, I leaned more towards the idea of allowing the piece creator the freedom to choose what they prefer. Yet, I realize that for the end user, this could be confusing in some cases if separators are used without a common logic.

If you believe that it would be better to keep only the groups, I am completely willing to proceed in that manner. I will then remove the other two options without any issue!

pieces-form-recusive

For more precision here is the code for the corresponding part

import {createAction, Property, PropertyStyle} from "@activepieces/pieces-framework";

export const demoAction = createAction({
    name: 'demo',
    displayName: "Demo",
    description: "This is a demo action",
    props: {
        user: PropertyStyle.Group({
            displayName: 'User',
            props: {
                name: Property.ShortText({
                    displayName: 'Name',
                    description: 'The name of the member',
                    required: true,
                }),
                sep: PropertyStyle.Separator(),
                email: Property.ShortText({
                    displayName: 'Email',
                    description: 'The email of the member',
                    required: true,
                }),
                security: PropertyStyle.Title({
                    displayName: 'Security',
                }),
                password: Property.ShortText({
                    displayName: 'Password',
                    description: 'The password of the member',
                    required: true,
                })
            }
        }),
    },
    async run (context) {

    }
})

@Doskyft Doskyft marked this pull request as ready for review November 21, 2023 14:51
@abuaboud
Copy link
Contributor

@Doskyft Great perspective, Let's me try to get other team mates opinions and get back to you

I am being extra careful as these options are not easy to revert back once pieces start using it

@abuaboud abuaboud mentioned this pull request Mar 21, 2024
@abuaboud abuaboud added blocked dependent on another issue and removed 🤷‍♂️ action required Further action or information required labels Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked dependent on another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants