-
Notifications
You must be signed in to change notification settings - Fork 4
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: add runtime warning/errors to components #2007
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2007 +/- ##
==========================================
+ Coverage 97.53% 97.58% +0.05%
==========================================
Files 108 109 +1
Lines 2555 2572 +17
Branches 640 645 +5
==========================================
+ Hits 2492 2510 +18
+ Misses 61 60 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
e71034a
to
296218c
Compare
@@ -7,6 +7,17 @@ import * as stories from './Button.stories'; | |||
import type { StoryFile } from '../../util/utility-types'; | |||
|
|||
describe('<Button />', () => { | |||
beforeEach(() => { | |||
const consoleMock = jest.spyOn(console, 'error'); | |||
const consoleWarnMock = jest.spyOn(console, 'warn'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is consoleWarnMock
used to hide warning messages while tests run? I don't see it used apart from consoleWarnMock.mockImplementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good callout. I forgot to add an expect
to make sure this isn't called at all .toHaveBeenCalledTimes(0)
typeof isDisabled === 'undefined' && | ||
typeof other.disabled !== 'undefined', | ||
], | ||
'Use "isDisabled" instead of "disabled" on button instances', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more context, what cases is this warning for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be when using disabled
instead of isDisabled
, that situation where the UI shows that red dotted outline around a button (e.g., this case)
Instead of just a visual warning, this now will include some console warnings (or errors)
- add utility function for doing the logging - use in various components (Link, Button, util.s) - fix violations in EDS identified by these checks - add additional tests
296218c
to
b8771ee
Compare
@jeremiah-clothier thanks ! |
## [15.1.0](v15.0.1...v15.1.0) (2024-07-15) [Storybook](https://61313967cde49b003ae2a860-qztphlqyid.chromatic.com/) ### Features * add runtime warning/errors to components ([#2007](#2007)) ([661130b](661130b)) * **InputField:** add show/hide button for password field type ([#2006](#2006)) ([52d9ca0](52d9ca0)) * **Modal:** add height property to modal API ([#2011](#2011)) ([8d0c68f](8d0c68f)) ### Bug Fixes * **Icon:** update pencil icon to latest design ([#2016](#2016)) ([cb8d1a7](cb8d1a7)) * **Link:** apply font weight to standalone sizes ([#2015](#2015)) ([2e47271](2e47271)) * **Select:** expose generic types to allow by to pass type checks ([#2008](#2008)) ([421c91b](421c91b))
Test Plan: