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

Add guard-clause to clearDirty method #16404

Merged
merged 3 commits into from May 23, 2023
Merged

Conversation

sebastian-marinescu
Copy link
Contributor

What does it do?

This makes sure the JS doesn't throw an error in the case that the items-object is an array.

Why is it needed?

When using ExtJS to create components of the xtype: 'radiogroup' to be used in CMPs inside a form,
this function gets called and tries to iterate over it's items array with a method that doesn't exist.

This PR isn't changing the way forms or the clearDirty-method are handled, it just assures no error is thrown when using radiogroup-components.

Related issue(s)/PR(s)

Resolves #12177
Resolves modmore/ClientConfig#202
Resolves modmore/ClientConfig#176
Resolves modmore/ClientConfig#143

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Feb 27, 2023
@Ibochkarev Ibochkarev added the pr/review-needed Pull request requires review and testing. label Feb 27, 2023
Copy link
Collaborator

@smg6511 smg6511 left a comment

Choose a reason for hiding this comment

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

For the simple purpose of avoiding error, this does the job. Supporting checkboxgroup/radiogroup types may need to be revisited, as it's hard to know whether the clearDirty method is taking effect on those component types without setting up a special CMP for the purpose of testing.

@JoshuaLuckers JoshuaLuckers added this to the v3.0.4 milestone Mar 17, 2023
@Ibochkarev Ibochkarev added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels May 15, 2023
@opengeek opengeek added the requires build Grunt build is required for integration label May 23, 2023
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch coverage has no change and project coverage change: -3.84 ⚠️

Comparison is base (b16a856) 21.73% compared to head (5dedcc8) 17.90%.

❗ Current head 5dedcc8 differs from pull request most recent head 105a633. Consider uploading reports for the commit 105a633 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16404      +/-   ##
============================================
- Coverage     21.73%   17.90%   -3.84%     
+ Complexity    10483    10480       -3     
============================================
  Files           561      561              
  Lines         31614    39240    +7626     
============================================
+ Hits           6872     7024     +152     
- Misses        24742    32216    +7474     

see 477 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@opengeek opengeek merged commit 39a5672 into modxcms:3.x May 23, 2023
5 checks passed
opengeek added a commit that referenced this pull request May 23, 2023
@sebastian-marinescu
Copy link
Contributor Author

Very nice! 🦾

opengeek added a commit that referenced this pull request Mar 22, 2024
### What does it do?
This makes sure the JS doesn't throw an error in the case that the
items-object is an array, just like in
#16404 for 3.x

### Why is it needed?
When using ExtJS to create components of the xtype: 'radiogroup' to be
used in CMPs inside a form,
this function gets called and tries to iterate over it's items array
with a method that doesn't exist.

This PR isn't changing the way forms or the clearDirty-method are
handled, it just assures no error is thrown when using
radiogroup-components.

### Related issue(s)/PR(s)
Resolves modmore/ClientConfig#202
Resolves modmore/ClientConfig#176
Resolves modmore/ClientConfig#143

---------

Co-authored-by: Jason Coward <jason@opengeek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/ready-for-merging Pull request reviewed and tested and ready for merging. requires build Grunt build is required for integration
Projects
None yet
5 participants