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

fix: suppress warnings from useId imports and prop usage #1606

Merged
merged 2 commits into from
May 2, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented May 2, 2023

Summary:

  • move the console.warn inside the function usage (TODO: consider deduping its output)
  • use proper camelCase prop name in textarea 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.

@booc0mtaco booc0mtaco requested a review from a team May 2, 2023 20:20
@github-actions
Copy link

github-actions bot commented May 2, 2023

size-limit report 📦

Path Size
components 94.5 KB (+0.03% 🔺)
styles 36.61 KB (0%)

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #1606 (70a2dbe) into next (4d32194) will decrease coverage by 0.07%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             next    #1606      +/-   ##
==========================================
- Coverage   91.39%   91.32%   -0.07%     
==========================================
  Files         280      280              
  Lines        4193     4195       +2     
  Branches      787      787              
==========================================
- Hits         3832     3831       -1     
- Misses        335      338       +3     
  Partials       26       26              
Impacted Files Coverage Δ
...components/TextareaField/TextareaField.stories.tsx 100.00% <ø> (ø)
src/util/useId.ts 38.46% <25.00%> (-16.09%) ⬇️

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

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

So this message will potentially be logged a lottt more now? Is that a concern

@@ -15,7 +15,7 @@ export default {
label: 'Textarea Field',
rows: 5,
fieldNote: 'Longer Field description',
spellcheck: false,
Copy link

Choose a reason for hiding this comment

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

surprised this wasn't a typescript error before. Would is have been if you changed as Meta<Args> to satisfies Meta<Args>?

Copy link
Contributor Author

@booc0mtaco booc0mtaco May 2, 2023

Choose a reason for hiding this comment

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

it's possible. I can test that out. It's also possible typescript might allow both, and only warns at runtime... which react does for some kinds of things (the lower-case prop actually does what it says it should)

EDIT: using satisfies works better ! :) it complains when I use the lowercase one. It would be a good tidying task to move all of the tests over to that syntax at some point.. also looks like i lied :( i thought it was applying the prop but it updated the snapshots this time along with the chromatic snaphots

@booc0mtaco
Copy link
Contributor Author

So this message will potentially be logged a lottt more now? Is that a concern

Thinking about the browser case, the messages would collapse under a counted set instead of printing all on separate rows if it printed multiple times. I can add a boolean toggle at the higher scope to print this exactly once, since it could become obnoxious.

@booc0mtaco booc0mtaco merged commit c875d9d into next May 2, 2023
@booc0mtaco booc0mtaco deleted the aholloway/x-fix-test-warnings branch May 2, 2023 22:40
@booc0mtaco booc0mtaco mentioned this pull request May 2, 2023
booc0mtaco added a commit that referenced this pull request May 2, 2023
### [12.0.1](v12.0.0...v12.0.1) (2023-05-02)

### Bug Fixes

* polyfill for useid for react <18 ([#1595](#1595)) ([4d32194](4d32194))
* restore check for undefined any types ([#1600](#1600)) ([214cd88](214cd88))
* suppress warnings from useId imports and prop usage ([#1606](#1606)) ([c875d9d](c875d9d))
* **Typography:** update type ramp to sync with design  ([#1601](#1601)) ([be5b868](be5b868))
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.

1 participant