-
Notifications
You must be signed in to change notification settings - Fork 142
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(BigNumbers): Convert to TypeScript #6734
base: main
Are you sure you want to change the base?
fix(BigNumbers): Convert to TypeScript #6734
Conversation
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
} | ||
|
||
.#{$block-class}__info { | ||
vertical-align: top; | ||
} | ||
|
||
.#{$block-class}__trend { | ||
margin-top: $spacing-03; | ||
margin-block-start: $spacing-03; |
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.
Added a little space so that the button isn't touching the number on hover.
@@ -32,44 +34,37 @@ const numericOptions = { | |||
'undefined ': undefined, | |||
}; | |||
|
|||
const iconButtonOptions = { |
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.
Added the ability to show/hide the Edit button via a Storybook dropdown. This means we could remove the withEditButton
page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6734 +/- ##
==========================================
+ Coverage 80.90% 80.97% +0.06%
==========================================
Files 398 399 +1
Lines 12953 12958 +5
Branches 4280 4280
==========================================
+ Hits 10480 10493 +13
+ Misses 2473 2465 -8
|
trending: false, | ||
truncate: true, | ||
locale: 'en-US', | ||
value: 5, |
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.
Fixed value
so that it's pointing to an item in the list by default.
kind="ghost" | ||
size={'sm'} | ||
hasIconOnly | ||
onClick={action('Button.onClick()')} |
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.
Added an action
so that clicking the Edit button shows an entry in the Actions tab when clicked.
@@ -157,20 +173,3 @@ export const bigNumbers = Template.bind({}); | |||
bigNumbers.args = { | |||
...defaultProps, | |||
}; | |||
|
|||
export const withEditButton = Template.bind({}); |
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.
With the added ability to show/hide the Edit button in the Controls tab, this template is no longer req'd.
total={55555} | ||
percentage={false} | ||
size={BigNumbersSize.Default} | ||
forceShowTotal={false} | ||
trending={false} | ||
truncate={true} | ||
locale="en-US" |
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.
Removing these "default" values means each test below only has to pass the props specific to the test, instead of having to consider "add/change/remove" every time.
import { TooltipTrigger } from '../TooltipTrigger'; | ||
import { BigNumbersSkeleton } from './BigNumbersSkeleton'; |
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.
The skeleton component that was sitting at the bottom of this file was moved to its own file.
@@ -184,126 +180,64 @@ BigNumbers.displayName = componentName; | |||
// in alphabetical order (for consistency). | |||
// See https://www.npmjs.com/package/prop-types#usage. | |||
BigNumbers.propTypes = { | |||
/** | |||
* Optional class name. | |||
* @type number |
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.
Removed all BigNumbers.propTypes
> @type ...
references since it doesn't seem to be used elsewhere.
Intl.NumberFormat.supportedLocalesOf(locale).length > 0 | ||
? locale | ||
: DefaultLocale; | ||
export const getSupportedLocale = (locale) => { |
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.
Updated to try/catch
because this
Intl.NumberFormat.supportedLocalesOf(locale);
will throw an error if an unsupported locale string is used.
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 looks like a handy utility. i would move this into the global utilities folder!
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.
looking good! just a few comments
}, | ||
ref | ||
}: BigNumbersProps, | ||
ref: React.ForwardedRef<HTMLDivElement> | ||
) => { |
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.
minor stylistic thing here, but i think it's cleaner to adopt this signature
const Component = forwardRef<HTMLDivElement, Props>((props, ref,) => {
const { prop1 = 'default value', prop2, prop3, ...rest } = props;
...
});
this way you get more stuff out of the signature and into the body
// Use the same properties and values as parent BigNumbersProps | ||
type BigNumbersSkeletonProps = Pick<BigNumbersProps, 'className' | 'size'>; | ||
|
||
export const BigNumbersSkeleton = React.forwardRef( |
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.
same comment about signature structure
return ( | ||
<div | ||
{...rest} | ||
className={cx(className, blockClass, BigNumbersSkeletonClasses)} |
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.
you can just include className
and blockClass
in the BigNumbersSkeletonClasses
definition and simplify it to className={BigNumbersSkeletonClasses}
|
||
import { getDevtoolsProps } from '../../global/js/utils/devtools'; | ||
import { pkg } from '../../settings'; | ||
import classnames from 'classnames'; |
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.
cx
is already being imported above
Intl.NumberFormat.supportedLocalesOf(locale).length > 0 | ||
? locale | ||
: DefaultLocale; | ||
export const getSupportedLocale = (locale) => { |
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 looks like a handy utility. i would move this into the global utilities folder!
Closes #6689
What did you change?
How did you test and verify your work?