You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aobapirak (via GitHub)" <gi...@apache.org> on 2024/01/29 10:57:00 UTC
[PR] feat: Custom server page length [superset]
aobapirak opened a new pull request, #26847:
URL: https://github.com/apache/superset/pull/26847
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This contribution delivers usability enhancements to the Custom Server Page Length feature in Superset. This upgrade now enables custom input values to persist in the drop-down list. The previous version of this feature posed an inconvenience as any new custom value entered by the user would disappear from the drop-down list when the user moved to a different choice.
The new implementation, however, ensures any custom value entered by the user remains in the drop-down list, thereby enriching user accessibility for frequent inputs. Moreover, the system now auto-arranges the drop-down list every time a new value is included, facilitating easy and efficient navigation.
Additional modifications include introducing a cap on the maximum custom page length value. The cap will be equivalent to either the total rows of data results or a fixed 10k rows in scenarios where the total rows of data exceed 10,000.
### BEFORE SCREENSHOTS OR ANIMATED GIF
https://github.com/apache/superset/assets/87610632/d6462d1f-1a74-4505-89f2-bf8f459147c5
### AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/apache/superset/assets/87610632/dbb1d509-5886-4452-a559-4bd465bb6e30
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Navigate to the Charts page
2. Choose any dataset
3. Choose table chart
4. Check the Server Pagination checkbox
5. Enter a new value in the selection drop-down list
6. Verify that the new input is sticky in the drop-down list
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1495527712
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
Review Comment:
Solved, I created MAX_SERVER_PAGE_LENGTH in consts.ts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1917583903
Thanks @aobapirak for the PR. Would you mind adding some tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1495530663
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] === formData.server_page_length,
+ )
+ ) {
+ formData.server_page_length_options?.push([
+ Number(formData.server_page_length),
+ formData.server_page_length === 0
+ ? 'All'
Review Comment:
I'm not sure what do you mean but I'm using the t function provided by @superset-ui/core as `t('All')` correct me if I'm wrong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1502939702
##########
superset-frontend/src/explore/components/controls/SelectControl.jsx:
##########
@@ -124,6 +124,17 @@ export default class SelectControl extends React.PureComponent {
onChangeVal = val[valueKey];
}
this.props.onChange(onChangeVal, []);
+
+ if (!this.state.options.some(option => option.value === onChangeVal)) {
+ this.setState(previousState => {
+ const newOptions = [
+ ...previousState.options,
+ { value: onChangeVal, label: onChangeVal },
+ ];
+ newOptions.sort((a, b) => a.value - b.value);
+ return { options: newOptions };
+ });
+ }
Review Comment:
@aobapirak can you confirm the need for this? To my understanding as long as the `allowNewOptions` for the Select is true, the component will allow for the creation of new options without having to rely on custom state?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1959629957
@aobapirak it seems you might need to update your branch with master in order for me to spin up a test environment for this PR. Would you mind doing that, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1957137185
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1959458185
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1504513288
##########
superset-frontend/src/explore/components/controls/SelectControl.jsx:
##########
@@ -124,6 +124,17 @@ export default class SelectControl extends React.PureComponent {
onChangeVal = val[valueKey];
}
this.props.onChange(onChangeVal, []);
+
+ if (!this.state.options.some(option => option.value === onChangeVal)) {
+ this.setState(previousState => {
+ const newOptions = [
+ ...previousState.options,
+ { value: onChangeVal, label: onChangeVal },
+ ];
+ newOptions.sort((a, b) => a.value - b.value);
+ return { options: newOptions };
+ });
+ }
Review Comment:
@aobapirak you are right that the options are not persisted unless selected. I had a different memory of this piece of the Select component. There is a problem of introducing this behaviour in the SelectControl wrapper as there is a risk of introducing inconsistency. Would you be open to have a wider discussion to make this a default behaviour of the Select component instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak closed pull request #26847: feat: Custom server page length
URL: https://github.com/apache/superset/pull/26847
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1928808581
Ping @john-bodley, could you please review? I have added some tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1952555284
Thanks for the contribution @aobapirak. Left a couple of comments. Looking forward to @michael-s-molina and @justinpark feedback as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1495531084
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] === formData.server_page_length,
+ )
+ ) {
+ formData.server_page_length_options?.push([
+ Number(formData.server_page_length),
+ formData.server_page_length === 0
+ ? 'All'
+ : String(formData.server_page_length),
+ ]);
+ formData.server_page_length_options?.sort(
+ (a: any[], b: any[]) => a[0] - b[0],
Review Comment:
Modified
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] === formData.server_page_length,
+ )
+ ) {
Review Comment:
Modified
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1503579790
##########
superset-frontend/src/explore/components/controls/SelectControl.jsx:
##########
@@ -124,6 +124,17 @@ export default class SelectControl extends React.PureComponent {
onChangeVal = val[valueKey];
}
this.props.onChange(onChangeVal, []);
+
+ if (!this.state.options.some(option => option.value === onChangeVal)) {
+ this.setState(previousState => {
+ const newOptions = [
+ ...previousState.options,
+ { value: onChangeVal, label: onChangeVal },
+ ];
+ newOptions.sort((a, b) => a.value - b.value);
+ return { options: newOptions };
+ });
+ }
Review Comment:
The new option doesn't stick to the options, i.e. whenever I enter a new option it will disappear when I add a new option, except for refreshing the page it will show all new options from this code:
`mapStateToProps: ({ controls }: ControlPanelState) => ({ choices: controls?.server_page_length_options?.value, })`
So the code above I created to allow SelectControl can be sticked new option to the options and sort them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1489711356
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] == formData.server_page_length,
Review Comment:
```suggestion
(option: [number, string]) => option[0] === formData.server_page_length,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1494618206
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] === formData.server_page_length,
+ )
+ ) {
+ formData.server_page_length_options?.push([
+ Number(formData.server_page_length),
+ formData.server_page_length === 0
+ ? 'All'
Review Comment:
I think this should be localized
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
Review Comment:
Can we move this to a constant for better visibility?
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] === formData.server_page_length,
+ )
+ ) {
Review Comment:
I think it would be better to improve readability of this logic by assigning some of these to constants
##########
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts:
##########
@@ -226,36 +226,61 @@ const transformProps = (
emitCrossFilters,
} = chartProps;
+ const [metrics, percentMetrics, columns] = processColumns(chartProps);
+
+ let baseQuery;
+ let countQuery;
+ let totalQuery;
+ let rowCount;
+ if (formData.server_pagination) {
+ [baseQuery, countQuery, totalQuery] = queriesData;
+ rowCount = (countQuery?.data?.[0]?.rowcount as number) ?? 0;
+ } else {
+ [baseQuery, totalQuery] = queriesData;
+ rowCount = baseQuery?.rowcount ?? 0;
+ }
+ const data = processDataRecords(baseQuery?.data, columns);
+
+ if (Number(formData.server_page_length) > 10000) {
+ formData.server_page_length = 10000;
+ }
+
+ if (
+ formData.server_pagination &&
+ !Number.isNaN(formData.server_page_length) &&
+ (formData.server_page_length || 0) <= data.length &&
+ !formData.server_page_length_options?.some(
+ (option: [number, string]) => option[0] === formData.server_page_length,
+ )
+ ) {
+ formData.server_page_length_options?.push([
+ Number(formData.server_page_length),
+ formData.server_page_length === 0
+ ? 'All'
+ : String(formData.server_page_length),
+ ]);
+ formData.server_page_length_options?.sort(
+ (a: any[], b: any[]) => a[0] - b[0],
Review Comment:
Is there a better type representation than `any`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1939455986
Thanks @aobapirak. Sorry I was out last week and thus wasn't able to respond. I've added a couple of frontend reviewers to your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1917624302
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `9 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`7c2093c`)](https://app.codecov.io/gh/apache/superset/commit/7c2093c495505a43a737ba298634a8e853bfaf5f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 67.16% compared to head [(`24e6de9`)](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 67.21%.
> Report is 56 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...d/plugins/plugin-chart-table/src/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL3RyYW5zZm9ybVByb3BzLnRz) | 11.11% | [5 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tend/plugins/plugin-chart-table/src/TableChart.tsx](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL1RhYmxlQ2hhcnQudHN4) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26847 +/- ##
==========================================
+ Coverage 67.16% 67.21% +0.05%
==========================================
Files 1894 1894
Lines 74175 74207 +32
Branches 8243 8249 +6
==========================================
+ Hits 49820 49880 +60
+ Misses 22286 22257 -29
- Partials 2069 2070 +1
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26847/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/26847/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <50.00%> (+0.10%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/26847?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "aobapirak (via GitHub)" <gi...@apache.org>.
aobapirak commented on code in PR #26847:
URL: https://github.com/apache/superset/pull/26847#discussion_r1503579790
##########
superset-frontend/src/explore/components/controls/SelectControl.jsx:
##########
@@ -124,6 +124,17 @@ export default class SelectControl extends React.PureComponent {
onChangeVal = val[valueKey];
}
this.props.onChange(onChangeVal, []);
+
+ if (!this.state.options.some(option => option.value === onChangeVal)) {
+ this.setState(previousState => {
+ const newOptions = [
+ ...previousState.options,
+ { value: onChangeVal, label: onChangeVal },
+ ];
+ newOptions.sort((a, b) => a.value - b.value);
+ return { options: newOptions };
+ });
+ }
Review Comment:
@geido The new option doesn't stick to the options, i.e. whenever I enter a new option it will disappear when I add a new option, except for refreshing the page it will show all new options from this code:
`mapStateToProps: ({ controls }: ControlPanelState) => ({ choices: controls?.server_page_length_options?.value, })`
So the code above I created to allow SelectControl can be sticked new option to the options and sort them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1961831172
@geido Ephemeral environment spinning up at http://54.218.80.169:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1961821833
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Custom server page length [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #26847:
URL: https://github.com/apache/superset/pull/26847#issuecomment-1952540197
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org