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

[UICAL-268, UICAL-228, UICAL-269, UICAL-220, UICAL-270] Convert to react-final-form-arrays, stripes-types, react 18 #491

Closed
wants to merge 35 commits into from

Conversation

ncovercash
Copy link
Member

@ncovercash ncovercash commented May 3, 2023

Jira UICAL-268 UICAL-228 UICAL-269 UICAL-220 UICAL-270

Purpose

This clean up a lot of technical debt with the previous way of handling form state. This should lead to a significantly improved experience, for both users and developers of the module. While in here, I also added some tests for TimeField (UICAL-228) and MCLRowFormatter (UICAL-220)

Approach

Before, a lot of the state management within react-final-form was not done optimally; this PR moves a lot of this logic into react-final-form and react-final-form-arrays' state management.

Other notes

  • A lot of the file difference here is from the removal of the old self-hosted types.
  • I also performed upgrades of most dependencies, as applicable, and purged some unused ones.
  • Some work will very likely need to be done to ensure this does not break platform-complete w.r.t. @types/react resolutions, etc.

@github-actions
Copy link

github-actions bot commented May 3, 2023

Jest Unit Test Statistics

    1 files  ±  0    70 suites   - 1   3m 55s ⏱️ +12s
261 tests  - 70  260 ✔️  - 70  1 💤 ±0  0 ±0 
282 runs   - 76  281 ✔️  - 76  1 💤 ±0  0 ±0 

Results for commit 746eea3. ± Comparison against base commit 0d6ad40.

This pull request removes 141 and adds 71 tests. Note that renamed tests count towards both.
 A closed row with days filled in is valid ‑  A closed row with days filled in is valid
 A closed row with nothing filled in is invalid ‑  A closed row with nothing filled in is invalid
 An open row with nothing filled in is invalid ‑  An open row with nothing filled in is invalid
 An open row with only days filled in is invalid ‑  An open row with only days filled in is invalid
 Bad ref inner closure rows are properly reported ‑  Bad ref inner closure rows are properly reported
 Bad ref inner opening row dates are properly reported ‑  Bad ref inner opening row dates are properly reported
 Conflicts are reported ‑  Conflicts are reported
 Date order is checked ‑  Date order is checked
 Date-time order is checked ‑  Date-time order is checked
 Empty errors are reported alone ‑  Empty errors are reported alone
…
 A non-filled entry does not split ‑  A non-filled entry does not split
 Closed exception missing any date value is not filled ‑  Closed exception missing any date value is not filled
 Closed rows are not considered ‑  Closed rows are not considered
 Default method works as expected ‑  Default method works as expected
 Error message is correct text ‑  Error message is correct text
 Exception with no rows nor name is considered filled ‑  Exception with no rows nor name is considered filled
 Exceptions convert properly ‑  Exceptions convert properly
 Exceptions with all date/time values is filled ‑  Exceptions with all date/time values is filled
 Issues are reported ‑  Issues are reported
 MCLRowFormatter renders ‑  MCLRowFormatter renders
…

♻️ This comment has been updated with latest results.

@ncovercash ncovercash requested review from nhanaa and GoZaddy May 3, 2023 16:37
@ncovercash
Copy link
Member Author

Also before y'all freak out at the number of line changes here: a lot of deletions are from the removal of the in-house stripes-components typings. Also, the rewriting of most all validation/calendar form tests made a pretty heavy impact, too.

@ncovercash ncovercash changed the title [UICAL-268, UICAL-228] Convert to react-final-form-arrays [UICAL-268, UICAL-228, UICAL-269] Convert to react-final-form-arrays May 3, 2023
@ncovercash ncovercash changed the title [UICAL-268, UICAL-228, UICAL-269] Convert to react-final-form-arrays [UICAL-268, UICAL-228, UICAL-269, UICAL-220] Convert to react-final-form-arrays May 3, 2023
@ncovercash ncovercash marked this pull request as draft May 3, 2023 18:23
@GoZaddy GoZaddy self-requested a review May 4, 2023 05:04
@sonarcloud
Copy link

sonarcloud bot commented May 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

98.7% 98.7% Coverage
0.0% 0.0% Duplication

@ncovercash ncovercash changed the title [UICAL-268, UICAL-228, UICAL-269, UICAL-220] Convert to react-final-form-arrays [UICAL-268, UICAL-228, UICAL-269, UICAL-220] Convert to react-final-form-arrays, stripes-types, react 18 Sep 22, 2023
@ncovercash ncovercash changed the title [UICAL-268, UICAL-228, UICAL-269, UICAL-220] Convert to react-final-form-arrays, stripes-types, react 18 [UICAL-268, UICAL-228, UICAL-269, UICAL-220, UICAL-270] Convert to react-final-form-arrays, stripes-types, react 18 Sep 22, 2023
@ncovercash ncovercash requested review from danetsao, a team, zburke and JohnC-80 September 22, 2023 20:28
@ncovercash ncovercash marked this pull request as ready for review September 22, 2023 20:29
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Gotta be honest: there is no practical way to review this PR. Big refactors like "leverage stripes-types" or "reformat for new lint rules" etc that make for really noisy diffs but don't actually change the logic much should be handled in their own PRs. As is, it's hard to separate these noisy-but-likely-safe changes from the must-be-closely-reviewed changes related to react-final-form-arrays.

I think a good way to proceed could be

  1. bump to react v18 and stripes-types
  2. react-final-form-arrays
  3. tests for TimeField, MCLRowFormatter (or maybe do this along with 2.)

I'm not gonna block it if you think the tests are sufficiently comprehensive, but I can't approve it either. Sorry.

@ncovercash
Copy link
Member Author

Gotta be honest: there is no practical way to review this PR. Big refactors like "leverage stripes-types" or "reformat for new lint rules" etc that make for really noisy diffs but don't actually change the logic much should be handled in their own PRs. As is, it's hard to separate these noisy-but-likely-safe changes from the must-be-closely-reviewed changes related to react-final-form-arrays.

I think a good way to proceed could be

  1. bump to react v18 and stripes-types
  2. react-final-form-arrays
  3. tests for TimeField, MCLRowFormatter (or maybe do this along with 2.)

I'm not gonna block it if you think the tests are sufficiently comprehensive, but I can't approve it either. Sorry.

Totally understood! It's a really noisy mess, no doubt about it. I'll try and separate out stripes-types vs. final form bits

@sonarcloud
Copy link

sonarcloud bot commented Oct 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

98.7% 98.7% Coverage
0.0% 0.0% Duplication

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.

4 participants