Enhancement/6243 loyal visitors #7126
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Build files for df6558f have been deleted. |
There was a problem hiding this comment.
Thanks @eugene-manuilov – this looks good to me, just a few points to consider.
| @@ -262,7 +296,7 @@ export const readableLargeNumber = ( number ) => { | |||
| * or a currency code. | |||
| * @return {string} The formatted number. | |||
| */ | |||
| export const numFmt = ( number, options = {} ) => { | |||
| export function numFmt( number, options = {} ) { | |||
There was a problem hiding this comment.
I prefer this form as well and I'm not sure if there is a better reason for one or the other, but we should add a linting rule for this to avoid various forms throughout.
There was a problem hiding this comment.
There is no benefits in using arrow functions for top level functions. IMO it just makes it harder to read. Created a ticket to add an eslint rule: #7146
| select( CORE_USER ).isKeyMetricsWidgetHidden() | ||
| ); | ||
|
|
||
| const dates = useSelect( ( select ) => | ||
| select( CORE_USER ).getDateRangeDates( { | ||
| offsetDays: DATE_RANGE_OFFSET, | ||
| compare: true, | ||
| } ) | ||
| ); | ||
| // One combined select hook is used to prevent unnecessary selects | ||
| // if the key metrics widget is hidden. | ||
| const [ report, loading ] = |
There was a problem hiding this comment.
I'm not against doing this kind of thing, but having the condition to return WidgetNull if KMW is hidden is something that each tile will need no? This seems like it would be better handled by an HOC (similar to whenActive for widgets), that way the inner component is never rendered / hooks invoked if the main requirement isn't met. WDYT?
There was a problem hiding this comment.
Good idea. Added a new HOC.
| // Prevent divide by zero errors. | ||
| if ( '0' === previous || 0 === previous || isNaN( previous ) ) { | ||
| if ( '0' === previous || 0 === previous || Number.isNaN( previous ) ) { |
There was a problem hiding this comment.
This looks like a good change, however the two are not identical in behavior. This should be covered by our tests but I wanted to highlight this:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isNaN#description
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist