feat(adoption-insights): add frontend plugin for adoption insights #464
Conversation
Eswaraiahsapram
requested review from
rohitkrai03 and
karthikjeeyar
as code owners
Changed Packages
|
…d api integration
…it tests and addressed review comments
…it tests and addressed review comments
|
@Eswaraiahsapram it looks amazing! I have a few questions:
|
…it tests and addressed review comments
…it tests and addressed review comments
@ShiranHi thanks for the feedback! I forgot to update the screenshots earlier. I'm adding a new screen recording now. Please let me know if anything needs to update. Regarding aligning the date picker width with the text fields, there are some limitations with the StaticDatePicker in MUI. However, I’ll explore possible adjustments to improve the alignment. |
…it tests and addressed review comments
|
Thanks for the video! I can now see more cards. I have a few follow-up questions:
|
| isYesterday, | ||
| } from 'date-fns'; | ||
|
|
||
| export const getDateRange = (value: string) => { |
There was a problem hiding this comment.
I feel we might be able put some of these into the common plugin once we have it.
|
There was a problem hiding this comment.
@Eswaraiahsapram This looks great 🎉, I have added few comments, PTAL.
FYI, Sonarcloud issues has to be fixed too.
| href={entityLink({ | ||
| kind: parseEntityRef(techdoc.entityRef)?.kind, | ||
| namespace: | ||
| parseEntityRef(techdoc.entityRef)?.namespace ?? | ||
| 'default', | ||
| name: parseEntityRef(techdoc.entityRef)?.name, | ||
| })} |
There was a problem hiding this comment.
index-page link takes you to non-existent page, for this index-page we should redirect to the home page of the docs plugin - /docs?filters%5Buser%5D=all&limit=20
| href={entityLink({ | ||
| kind: parseEntityRef(techdoc.entityRef)?.kind, | ||
| namespace: | ||
| parseEntityRef(techdoc.entityRef)?.namespace ?? | ||
| 'default', | ||
| name: parseEntityRef(techdoc.entityRef)?.name, |
There was a problem hiding this comment.
Refer my previous comment. since both of them using same logic extract it to a variable or function.
| <CardWrapper title="Top techdocs"> | ||
| <Box | ||
| display="flex" | ||
| justifyContent="center" | ||
| alignItems="center" | ||
| height={200} | ||
| > | ||
| <Typography>No results for this time range.</Typography> | ||
| </Box> | ||
| </CardWrapper> |
There was a problem hiding this comment.
Extract this into a component called EmptyChartState and reuse it in all the charts to avoid code duplication.
| alignItems="center" | ||
| height={200} | ||
| > | ||
| <Typography>No results for this time range.</Typography> |
There was a problem hiding this comment.
Should this be date range?
| <Typography>No results for this time range.</Typography> | |
| <Typography>No results for this date range.</Typography> |
| <TableCell sx={{ width: '20%' }}> | ||
| <Link | ||
| component="a" | ||
| href={`/${plugin.plugin_id}`} |
There was a problem hiding this comment.
I don't think we have a dedicated page for plugin, so this just take the user to a non-existent page. It is better to remove link and leave it as text.
| </div> | ||
| <Typography | ||
| variant="body2" | ||
| style={{ color: theme.palette.text.primary, fontSize: '14px' }} |
There was a problem hiding this comment.
please avoid px and use relative units (rem for fontsize) so it can scale up/down but px cant.
| style={{ color: theme.palette.text.primary, fontSize: '14px' }} | |
| style={{ color: theme.palette.text.primary, fontSize: '0.875rem' }} |
| <Box mr={3}> | ||
| <Typography | ||
| sx={{ | ||
| fontSize: '14px', |
There was a problem hiding this comment.
refer to my previous comment about using rem.
| <Button | ||
| variant="text" | ||
| startIcon={<FileDownloadOutlinedIcon />} | ||
| onClick={handleCSVDownload} |
There was a problem hiding this comment.
debounce this CSVDownload method, some users may double click the button and currently it calls the API and downloads it multiple times.
| handleDateRangeClick(event); | ||
| }} | ||
| > | ||
| Date range... |
There was a problem hiding this comment.
In figma design it reads Select from calendar...
| boxShadow: 4, | ||
| borderRadius: 2, | ||
| border: theme => `1px solid ${theme.palette.grey[300]}`, | ||
| backgroundColor: 'white', |
There was a problem hiding this comment.
This adds white color background in dark mode as well, so let us not set background color explicitly.
| backgroundColor: 'white', |
| ) : ( | ||
| visibleTemplates?.map(template => ( | ||
| <TableRow | ||
| key={template.entityRef} |
There was a problem hiding this comment.
I have changed this to lowercase in the response in this commit , Please rebase with main and align with that changes.
| key={template.entityRef} | |
| key={template.entityref} |
There was a problem hiding this comment.
The changes applies to Techdocs.tsx as well
| kind: parseEntityRef(template.entityRef).kind, | ||
| namespace: 'default', | ||
| name: parseEntityRef(template.entityRef)?.name, |
There was a problem hiding this comment.
nit:
parseEntityRef method is used multiple places in this file and in Techdocs.tsx, can you move it to a local variable in the top of files, so you can refer that variable to get kind and name information?.
| }, | ||
| }} | ||
| > | ||
| {parseEntityRef(template.entityRef).name} |
There was a problem hiding this comment.
parseEntityRef function can throw error and crash the UI if the entityRef is a empty object {} or null, so it better to add safety checks before calling this function.
Hey, I just made a Pull Request!
Fixes - RHDHPAI-571
This PR introduces frontend changes for the new Adoption Insights plugin, enhancing the experience by providing key insights into adoption metrics.
Changes Implemented
✅ Added UI components to display Adoption Insights metrics.
✅ Integrated data visualization using Recharts to present trends effectively.
✅ Integrated date range using MUI x-date-pickers
✅ Added Unit tests
✅ Implemented pagination.
Screen recording
Screen.Recording.2025-03-11.at.2.45.59.PM.mov
✔️ Checklist