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

CADENZA-33417 [Embedding FA] Parameters to control table contents #10

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

anistor
Copy link

@anistor anistor commented Oct 31, 2023

Add optional 'parts' parameter to #fetchData and #downloadData

https://jira.disy.net/browse/CADENZA-33417

@anistor anistor changed the title Slb/an/cadenza 33417 CADENZA-33417 [Embedding FA] Parameters to control table contents Oct 31, 2023
@anistor anistor requested a review from jkissel October 31, 2023 09:59
CHANGELOG.md Outdated Show resolved Hide resolved
src/cadenza.js Show resolved Hide resolved
src/cadenza.js Outdated Show resolved Hide resolved
src/cadenza.js Outdated Show resolved Hide resolved
src/cadenza.js Show resolved Hide resolved
src/cadenza.js Outdated Show resolved Hide resolved
src/cadenza.js Outdated Show resolved Hide resolved
@anistor anistor force-pushed the slb/an/CADENZA-33417 branch from 2c7eb37 to 592650d Compare November 3, 2023 05:39
@anistor
Copy link
Author

anistor commented Nov 3, 2023

I've addressed all comments added up to now. Thanks @jkissel !

@anistor anistor requested a review from jkissel November 3, 2023 05:43
Copy link
Member

@jkissel jkissel left a comment

Choose a reason for hiding this comment

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

Looks good now (y)

src/cadenza.js Outdated
@@ -608,6 +616,26 @@ function validGeometryType(/** @type string */ value) {
].includes(value);
}

/** @typedef {'columns' | 'values' | 'totals'} TablePart - A part of a table to export. */

const TablePart = /** @type {Record<string, TablePart>} */ {
Copy link
Member

Choose a reason for hiding this comment

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

This mapping is unused in Cadenza JS, an array of the values would be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Should the client code be able to use those constants? (if our code does not use them directly)

I can remove them just making sure it is really intended to obliterate them.
I see for geometries we do not have defined individual constants just an array of the possible values (as you suggest here) but for MediaTypes we have constants (which we also use in our code directly). Which of the 2 approaches is best?

Copy link
Member

Choose a reason for hiding this comment

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

The geometry types are used inside of the lib. If clients should be able to use this mapping, it needs to be exported and should be marked as @readonly and really made readonly using Object.freeze().

@anistor anistor force-pushed the slb/an/CADENZA-33417 branch from 592650d to 5ea8c57 Compare November 6, 2023 07:18
@anistor anistor force-pushed the slb/an/CADENZA-33417 branch from 5ea8c57 to 32cd8ba Compare November 10, 2023 06:58

const tableData = await response.json();
...
```
Copy link
Member

Choose a reason for hiding this comment

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

Nice example 👍

src/cadenza.js Outdated
@@ -608,6 +616,26 @@ function validGeometryType(/** @type string */ value) {
].includes(value);
}

/** @typedef {'columns' | 'values' | 'totals'} TablePart - A part of a table to export. */

const TablePart = /** @type {Record<string, TablePart>} */ {
Copy link
Member

Choose a reason for hiding this comment

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

The geometry types are used inside of the lib. If clients should be able to use this mapping, it needs to be exported and should be marked as @readonly and really made readonly using Object.freeze().

@anistor anistor merged commit 0fd4bcf into main Nov 10, 2023
1 check passed
@anistor anistor deleted the slb/an/CADENZA-33417 branch November 10, 2023 10:01
@anistor
Copy link
Author

anistor commented Nov 10, 2023

Thanks @jkissel !

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.

2 participants