-
Notifications
You must be signed in to change notification settings - Fork 99
feat: convert ProgressBar component to TypeScript #4139
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
base: release-23.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import React from 'react'; | |
| import { render } from '@testing-library/react'; | ||
| import renderer from 'react-test-renderer'; | ||
|
|
||
| import ProgressBar, { ANNOTATION_CLASS } from '..'; | ||
| import ProgressBar, { ANNOTATION_CLASS, ProgressBarAnnotatedProps } from '..'; | ||
|
|
||
| const ref = { | ||
| current: { | ||
|
|
@@ -24,9 +24,9 @@ const ref = { | |
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| } as any; | ||
|
|
||
| function ProgressBarElement(props) { | ||
| function ProgressBarElement(props: ProgressBarAnnotatedProps) { | ||
| return ( | ||
| <ProgressBar.Annotated | ||
| now={20} | ||
|
|
@@ -99,13 +99,13 @@ describe('<ProgressBar.Annotated />', () => { | |
| expect(progressHints[1].textContent).toEqual(''); | ||
| }); | ||
| it('should apply styles based on direction for threshold', () => { | ||
| window.getComputedStyle = jest.fn().mockReturnValue({ getPropertyValue: () => 'rtl' }); | ||
| window.getComputedStyle = jest.fn().mockReturnValue({ getPropertyValue: () => 'rtl' }) as any; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use jest.spyOn with the mock implementation instead of assigning this property and casting it to any? |
||
| const { container } = render(<ProgressBarElement />); | ||
| const progressInfo = container.querySelector('.pgn__progress-info'); | ||
| const computedStyles = window.getComputedStyle(progressInfo); | ||
| const computedStyles = window.getComputedStyle(progressInfo!); | ||
|
|
||
| expect(computedStyles.getPropertyValue('directory')).toBe('rtl'); | ||
| window.getComputedStyle.mockRestore(); | ||
| (window.getComputedStyle as jest.Mock).mockRestore(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return value of spyOn should be the right type. |
||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ const ref = { | |
| }, | ||
| ], | ||
| }, | ||
| }; | ||
| } as any; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This structure is used in two places but is cast to any. Can we find a way to type it properly so it's used correctly in both places? Other concern-- this isn't part of your PR but seems like something we should clean up while we're here: The object is created once but is used in multiple tests. That means it could change between tests and any subtle errors from this would be difficult to detect. Could you create a function which returns this object instead and use that for each test? |
||
|
|
||
| describe('utils', () => { | ||
| describe('placeInfoAtZero', () => { | ||
|
|
@@ -86,8 +86,8 @@ describe('utils', () => { | |
| expect(actualMarginRight).toEqual(expectedHorizontalMargin); | ||
| }); | ||
| it('returns false if reference is wrong', () => { | ||
| const wrongRef1 = {}; | ||
| const wrongRef2 = { current: {} }; | ||
| const wrongRef1 = {} as any; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing these are |
||
| const wrongRef2 = { current: {} } as any; | ||
| expect(placeInfoAtZero(wrongRef1)).toEqual(false); | ||
| expect(placeInfoAtZero(wrongRef2)).toEqual(false); | ||
| }); | ||
|
|
||
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.
We've got two files where this type is used. Could we get a type annotation here? Also noticed that this is a bare object but should be created via function (see my other comment on a similar line)