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: add row in action forms #1173

Merged
merged 13 commits into from
Sep 18, 2024
Merged

feat: add row in action forms #1173

merged 13 commits into from
Sep 18, 2024

Conversation

EnkiP
Copy link
Member

@EnkiP EnkiP commented Sep 13, 2024

Definition of Done

CU-86c097bm3

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@forest-bot
Copy link
Member

@EnkiP EnkiP force-pushed the feat/add-row-in-action-forms branch from 3762841 to 1953b49 Compare September 17, 2024 09:08
Copy link

codeclimate bot commented Sep 18, 2024

Code Climate has analyzed commit 79f503b and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 98.0% (98% is the threshold).

This pull request will bring the total coverage in the repository to 97.5% (0.0% change).

View more on Code Climate.

return element;
}

allFields.push(element);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢 Mutating function parameters. 😬

Comment on lines -136 to -144
{
type: 'Layout',
component: 'Separator',
},
{
type: 'Layout',
component: 'HtmlBlock',
content: 'some text content',
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

field[subElementsKey] = await handler(field[subElementsKey] || []);
}

private async copyFields(fields: DynamicFormElement[]) {
Copy link
Member

@Thenkei Thenkei Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this need a little explaination.

Because,

  • it copy the fields but also do crazy things (async + call handler on subfields) ?
  • I don't understand the need of async in there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, you use this mechanism to do async work dropDefault (= compute default value) , dropIfs (= remove field that have if: false), ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But style copyFields should be sync.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can define two versions of executeOnSubFields

Copy link
Member

@Thenkei Thenkei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • ✅ Code review 💪 (some tiny improvments will come in a refactor PR)
    • ✅  Use pure functions when possible
    • ✅  Performance concerns
    • ✅  The PR tackle one subject only
  • ✅ Automatic tests
    • ✅  Unit tests
    • ✅  Integration tests
  • ✅ Manual tests
  • ✅ PR title
  • ❔ PR linked to the clickup task

@EnkiP EnkiP merged commit 8770699 into main Sep 18, 2024
22 checks passed
@EnkiP EnkiP deleted the feat/add-row-in-action-forms branch September 18, 2024 13:47
forest-bot added a commit that referenced this pull request Sep 18, 2024
@forest-bot
Copy link
Member

🎉 This PR is included in version 1.3.17 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.9.0 🎉

The release is available on example@1.9.0

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.44.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.50.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.1.28 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.7.17 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants