Skip to content
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

[MIM-1947] Mim 1947 table export refactor #3073

Merged
merged 12 commits into from
Dec 20, 2024

Conversation

johnnadeluy
Copy link
Contributor

@johnnadeluy johnnadeluy commented Dec 18, 2024

Jira Issues!

Link to ticket: MIM-1947

  • Scrap tableExport jquery plugin and use SheetJS utils for Table elements instead
  • Host SheetJS tarball package ourselves and install from /lib/vendor
    • Using v.0.20.3 of xlsx, which will get rid of "Prototype Pollution in sheetJS" vulnerability
  • Add feature toggling for old Table Export and new Table Export for testing
  • Use Table caption for fileName instead of generic "tabell" name

@github-actions github-actions bot changed the title Mim 1947 table export refactor [MIM-1947] Mim 1947 table export refactor Dec 18, 2024
@johnnadeluy johnnadeluy marked this pull request as ready for review December 18, 2024 10:46
michaelpande
michaelpande previously approved these changes Dec 18, 2024
Copy link
Contributor

@michaelpande michaelpande left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeg har ikke testet koden, men lest igjennom.
Har ikke så mange kommentarer, men noen. Fiks de du er enig i 😄

Edit: Fjernet spørsmål om hva som har skjedd med den gamle tableExport, ser at det er brukt featureToggle :)

@@ -56,7 +76,7 @@ function Table(props: TableProps) {
}

function addDownloadTableDropdown(mobile: boolean) {
if (props.downloadTableLabel && props.downloadTableTitle && props.downloadTableOptions) {
if (downloadTableLabel && downloadTableTitle && downloadTableOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vurder å bruk dette som en if-guard istedenfor.

if(!downloadTableLabel || !downloadTableTitle || !downloadTableOptions) {
return null;
}

Comment on lines 384 to 390
if (showPreviewDraft) {
if (fetchUnPublished && draftExist) {
return <Alert variant='info'>Tallene i tabellen nedenfor er upublisert</Alert>
} else if (fetchUnPublished && !props.draftExist) {
} else if (fetchUnPublished && !draftExist) {
return <Alert variant='warning'>Finnes ikke upubliserte tall for denne tabellen</Alert>
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Her er fetchUnpublished brukt to ganger.

Vurder å skriv det om til:

if(showPreviewDraft && fetchUnpublished) {
return draftExists? <Alert.....> : <Alert.....>
}

return <Alert variant='warning'>Finnes ikke upubliserte tall for denne tabellen</Alert>
}
}
return null
}

function renderSources() {
if ((props.sourceListTables && props.sourceListTables.length > 0) || (props.sources && props.sources.length > 0)) {
if ((sourceListTables && sourceListTables.length > 0) || (sources && sources.length > 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vurder å konverter dette til en if-guard. Slik at den returnerer null ved det motsatte logiske utrykket. Da slipper vi å nøste så mye kode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeg fikk ikke til dette helt med motsatt logiske utrykk men det gikk å forkorte det til
if (sourceListTables?.length || sources?.length) heldigvis

ssb-cgn
ssb-cgn previously approved these changes Dec 19, 2024
Copy link
Contributor

@ssb-cgn ssb-cgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good 👍

@johnnadeluy johnnadeluy dismissed stale reviews from ssb-cgn and michaelpande via c8e42a7 December 19, 2024 08:11
@johnnadeluy johnnadeluy merged commit 3a83cdf into develop Dec 20, 2024
11 checks passed
@johnnadeluy johnnadeluy deleted the MIM-1947_table_export_refactor branch December 20, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants