#1367: Added Recent Artists Pie Chart to ListeningHistory #1599
Conversation
alicemmartinez
changed the base branch from
feature/listening-history
to
master
|
| ]; | ||
|
|
||
| // Function to get the top N items from the data based on a given key | ||
| const getTopN = (data: any[], key: string, n: number): any[] => { |
There was a problem hiding this comment.
Make this function better by making it generic (<T>), instead if using any. That way we can ensure type safety for its input and output.
| data: any[]; | ||
| } | ||
|
|
||
| const timeframes = ['Last 7 days', 'Last 30 days', 'Last 90 days', 'Last 180 days', 'Last 365 days', 'All time']; |
There was a problem hiding this comment.
Nuclear is translated into more than 30 languages. So we should never use hardcoded strings like these, and instead use react-i18next. Try looking for the useTranslation hook to see how it's used.
If you decide to move this component to the ui package, you can simply feed it strings it needs to render its interface. For an example of how it can be done, you can look for TrackPopupStrings. The idea is that the component in ui takes a list of texts it's going to render, and in the app package, when we want to use that component, we look up the translated texts using the useTranslation hook. If you're unsure how to do this, let me know.
There was a problem hiding this comment.
And the same comment applies to other instances of raw text, like Timeframe, etc.
| </select> | ||
| </label> | ||
|
|
||
| <label style={{ marginLeft: '20px' }}> |
There was a problem hiding this comment.
Instead of using the style attribute, try using scss files and classes, it separates the styles from the markup.
| @@ -0,0 +1,55 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
The convention for tests is to use <container>.test.ts.
| ]; | ||
|
|
||
| describe('PieChartTopArtists', () => { | ||
| test('renders without crashing', () => { |
There was a problem hiding this comment.
This can be replaced with a snapshot test, which gives us the same information, and also records what the component looks like in the HTML.
nukeop
added
the
needs changes
Have not written test cases, or completely implemented filtering algorithm for range of dates. I was hoping you could guide me to where I could write test cases for my added subcomponent?