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(InputField): support recommendedMaxLength prop for display-only errors #1771

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Oct 2, 2023

Summary:

  • allow handling for maxlength and recommendedMaxLength like TextareaField
  • update snapshots and tests

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:
    • Create an alpha publish and try out in edu-stack or traject as a sanity check if changes affect build or deploy, or are breaking, such as token changes, widely used component updates, hooks changes, and major dependency upgrades.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #1771 (0d92cbf) into next (0852356) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             next    #1771      +/-   ##
==========================================
+ Coverage   92.29%   92.35%   +0.06%     
==========================================
  Files         146      147       +1     
  Lines        2635     2656      +21     
  Branches      697      711      +14     
==========================================
+ Hits         2432     2453      +21     
  Misses        187      187              
  Partials       16       16              
Files Coverage Δ
src/components/InputField/InputField.tsx 100.00% <100.00%> (ø)
src/components/TextareaField/TextareaField.tsx 96.92% <100.00%> (-0.27%) ⬇️
src/util/getMinValue.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@booc0mtaco
Copy link
Contributor Author

Screenshot 2023-10-02 at 16 17 41

@booc0mtaco booc0mtaco marked this pull request as ready for review October 2, 2023 22:19
@booc0mtaco booc0mtaco added the do not merge PRs that should not be merged (even if the build is green or there are approvals) label Oct 2, 2023
@booc0mtaco booc0mtaco requested a review from a team October 2, 2023 22:19
Comment on lines +191 to +192
const textExceedsMaxLength =
maxLength !== undefined && fieldLength ? fieldLength > maxLength : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, couple ideas:

const fieldLength = fieldText?.toString().length ?? 0;
const textExceedsMaxLength = maxLength && fieldLength > maxLength;

or give maxLength a default and simplifyt the check even more

maxLength = Infinity

...

const textExceedsMaxLength = fieldLength > maxLength;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will apply a few things where possible. One thing I wanted to avoid were any props being added if the consumer didn't specify them. maxLength is also weird in that it changes how the base field behaves when it has a value, and it treats Infinity as invalid if it gets to <textarea>

);

// Pick the smallest of the lengths to set as the maximum value allowed
const maxLengthShown = getMinValue(maxLength, recommendedMaxLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

If maxLength and recommendedMaxLength are numbers (e.g. if they have default values or fallbacks), may be able to use Math.min?

Base automatically changed from aholloway/EFI-1382 to next October 3, 2023 02:31
…errors

- allow handling for maxlength and recommendedMaxLength like
  TextareaField
- add utility method for finding smallest value
- update snapshots and tests
@booc0mtaco booc0mtaco removed the do not merge PRs that should not be merged (even if the build is green or there are approvals) label Oct 3, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

size-limit report 📦

Path Size
components 97.09 KB (+0.63% 🔺)
styles 32.86 KB (+0.2% 🔺)

@booc0mtaco booc0mtaco merged commit cc84a20 into next Oct 3, 2023
8 checks passed
@booc0mtaco booc0mtaco deleted the aholloway/EFI-1381 branch October 3, 2023 15:12
@booc0mtaco booc0mtaco mentioned this pull request Oct 3, 2023
booc0mtaco added a commit that referenced this pull request Oct 4, 2023
## [13.4.0](v13.3.0...v13.4.0) (2023-10-03)


### Features

* **Accordion:** allow empty or hidden accordion rows ([#1767](#1767)) ([e044a85](e044a85))
* **Icons:** allow component icon usages to be headless ([#1761](#1761)) ([ba454bf](ba454bf))
* **InputField:** support recommendedMaxLength prop for display-only errors ([#1771](#1771)) ([cc84a20](cc84a20))
* **Tabs:** add ability to customize tab button headers ([#1768](#1768)) ([f231ad4](f231ad4))
* **TextareaField:** support recommendedMaxLength prop for display-only errors ([#1769](#1769)) ([0852356](0852356))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants