NP-50911 Use link instead of button for export#8336
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pages/messages/components/ExportNviStatusLink.tsx (1)
8-10: Interface name does not match the component name.The interface is named
ExportNviStatusButtonPropsbut the component isExportNviStatusLink. Consider renaming for consistency.Suggested rename
-interface ExportNviStatusButtonProps { +interface ExportNviStatusLinkProps { acronym: string; } -export const ExportNviStatusLink = ({ acronym }: ExportNviStatusButtonProps) => { +export const ExportNviStatusLink = ({ acronym }: ExportNviStatusLinkProps) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/messages/components/ExportNviStatusLink.tsx` around lines 8 - 10, Rename the props interface to match the component name: change ExportNviStatusButtonProps to ExportNviStatusLinkProps (or alternatively rename the component to ExportNviStatusButton) and update all references accordingly so the component ExportNviStatusLink uses the matching interface name; ensure imports/exports and any type annotations (e.g., function signature or React.FC<...>) reference the new ExportNviStatusLinkProps identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/NviPublicationPointsTexts.tsx`:
- Line 51: The export link is always rendered because exportAcronym is coerced
to '' and passed into ExportNviStatusLink; update the rendering logic in
NviPublicationPointsTexts (the object that contains link: <ExportNviStatusLink
... />) to check exportAcronym first and either render a placeholder/disabled
component or an alternative text when exportAcronym is undefined, or instead
remove the fallback and let ExportNviStatusLink handle empty/undefined values
(update ExportNviStatusLink to early-return a disabled link or null when acronym
is falsy); reference the exportAcronym variable and the ExportNviStatusLink
component to locate where to implement the conditional rendering or early-return
handling.
In `@src/pages/messages/components/ExportNviStatusLink.tsx`:
- Around line 16-20: The click handler calls
fetchNviApprovalReportQuery.refetch() even when acronym is empty, allowing a
manual API call with invalid params; update the handleClick in
ExportNviStatusLink to validate the acronym (derived from exportAcronym) before
calling fetchNviApprovalReportQuery.refetch() or early-return, and also
conditionally render/disable the link when acronym === '' so users cannot
trigger it; reference the handleClick function,
fetchNviApprovalReportQuery.refetch, exportAcronym prop and
useFetchNviReportExport hook when making the change.
---
Nitpick comments:
In `@src/pages/messages/components/ExportNviStatusLink.tsx`:
- Around line 8-10: Rename the props interface to match the component name:
change ExportNviStatusButtonProps to ExportNviStatusLinkProps (or alternatively
rename the component to ExportNviStatusButton) and update all references
accordingly so the component ExportNviStatusLink uses the matching interface
name; ensure imports/exports and any type annotations (e.g., function signature
or React.FC<...>) reference the new ExportNviStatusLinkProps identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82961bf0-6819-4a2e-b84d-ed101dc90d23
📒 Files selected for processing (7)
src/components/NviPublicationPointsTexts.tsxsrc/pages/messages/components/ExportNviStatusButton.tsxsrc/pages/messages/components/ExportNviStatusLink.tsxsrc/pages/messages/components/NviPublicationPointsPage.tsxsrc/pages/messages/components/NviStatusPage.tsxsrc/pages/messages/components/NviStatusWrapper.tsxsrc/translations/nbTranslations.json
💤 Files with no reviewable changes (3)
- src/pages/messages/components/NviStatusPage.tsx
- src/pages/messages/components/NviStatusWrapper.tsx
- src/pages/messages/components/ExportNviStatusButton.tsx
iBuzza
requested review from
Eiriknil,
denektenina and
joachimjorgensen
as code owners
denektenina
left a comment
denektenina
left a comment
There was a problem hiding this comment.
Looks good and integrates well with existing code. Could you answer my questions before I approve? Just want to make sure the choices are made on purpose.
| components={{ | ||
| p: <Typography />, | ||
| bold: <Typography component="span" sx={{ fontWeight: 'bold' }} />, | ||
| link: exportAcronym ? <ExportNviStatusLink acronym={exportAcronym} /> : <span />, |
There was a problem hiding this comment.
Possible to add a spinner here while loading, because there is no indicator that something happened after clicking the button (and the previous button had a spinner)
There was a problem hiding this comment.
Isn't this Button supposed to download something else soon? Should it maybe be disabled like in the design instead of deleted?
There was a problem hiding this comment.
Restored the button and made it only disabled for publication points
| opacity: fetchNviApprovalReportQuery.isFetching ? 0.7 : 1, | ||
| cursor: fetchNviApprovalReportQuery.isFetching ? 'default' : 'pointer', | ||
| }}> | ||
| <FileDownloadOutlinedIcon fontSize="small" /> |
There was a problem hiding this comment.
Is it agreed on that you should use a different icon than in the design?
There was a problem hiding this comment.
There is no icon for xls-file in MUI Icons, and I didn't really find a better option than the one already used. Suggestions are welcome, though
denektenina
left a comment
denektenina
left a comment
There was a problem hiding this comment.
Er det riktig at nedlastningsknappen skal fjernes fra rapporteringsstatus uten å erstattes av link? Går det an å gjøre så aggregation=all-kallet ikke gjøres før linken er klikket?
2 participants
Description
Link to Jira issue: https://sikt.atlassian.net/browse/NP-50911
Move the exporting of excel file from button to link.
How to test
Affected part of the application: http://localhost:3000/tasks/nvi/publication-points
Checklist
Ensure that the changes are aligned with the expectations of PO/designer before marking the PR as ready for review.
Also ensure that the following criterias are met:
Note: some of these criterias may not always be relevant, and can simply be marked as completed.
Summary by CodeRabbit
New Features
Refactor
Documentation