Skip to content

feat(form): implement an additional ngxs form plugin directive input #1840

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat(form): implement an additional ngxs form plugin directive input #1840

wants to merge 1 commit into from

Conversation

@rfprod
Copy link
Contributor

@rfprod rfprod commented May 11, 2022 •
edited
Loading

  • implement the ability to control the form.patchValue emitEvent option;
  • add unit test, review jest configs, update jest-preset-angular minor version;
  • update the form plugin documentation;

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1839

What is the new behavior?

The ngxs-form plugin directive now has an additional input ngxsFormPatchEmitEvent to control the form.patchValue method option emitEvent.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@rfprod rfprod force-pushed the ngxs-form-plugin-directive-inputs branch from 946c797 to 83cc589 Compare May 11, 2022 22:13
@rfprod rfprod changed the title feat(form): implemtn additional ngxs form plugin directive input feat(form): implement an additional ngxs form plugin directive input May 11, 2022
markwhitfeld
markwhitfeld requested changes May 21, 2022
View reviewed changes
Copy link
Member

@markwhitfeld markwhitfeld left a comment

Choose a reason for hiding this comment

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

Thank you for this very thorough PR!
I would like to hold off on merging this until we fully understand the need for this feature.
We would need to understand what use case it is addressing, if there are any workarounds, and how it would enhance the usage of the plugin.
A good place to do this would be in the issue you logged (#1839).

I notice that there are a number of adjustments you made to the jest setup.
If this is a fix to some of the test execution issues you had, then please feel free to split these particular changes into a separate PR to fix jest 👍 .

@markwhitfeld markwhitfeld marked this pull request as draft May 21, 2022 21:06
@rfprod rfprod force-pushed the ngxs-form-plugin-directive-inputs branch from 83cc589 to feefc29 Compare May 22, 2022 12:40
@rfprod
Copy link
Contributor Author

rfprod commented May 24, 2022

I notice that there are a number of adjustments you made to the jest setup. If this is a fix to some of the test execution issues you had, then please feel free to split these particular changes into a separate PR to fix jest +1 .

A separate PR with unit test changes #1846

@rfprod
feat(form): implement an additional ngxs form plugin directive input
cd548a6 
- [x] implement the ability to control the form.patchValue emitEvent option;
- [x] add unit test, review jest configs, update jest-preset-angular minor version;
- [x] update the form plugin documentation;
@rfprod rfprod force-pushed the ngxs-form-plugin-directive-inputs branch from feefc29 to cd548a6 Compare June 27, 2022 21:33
@arturovt
Copy link
Member

arturovt commented Aug 7, 2022

I'll close the PR (for now), since this feature needs discussion and needs some support from the community. At least if that user (in the Slack chat) was experiencing some weird bug, he could report a bug with a reproducible example provided.

@arturovt arturovt closed this Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markwhitfeld markwhitfeld markwhitfeld requested changes

@splincode splincode splincode approved these changes

@arturovt arturovt Awaiting requested review from arturovt arturovt is a code owner

@kuncevic kuncevic Awaiting requested review from kuncevic kuncevic is a code owner

@Carniatto Carniatto Awaiting requested review from Carniatto Carniatto is a code owner

@poloagustin poloagustin Awaiting requested review from poloagustin poloagustin is a code owner

@joaqcid joaqcid Awaiting requested review from joaqcid joaqcid is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rfprod @arturovt @markwhitfeld @splincode