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

[EuiBasicTable][EuiInMemoryTable][EuiDataGrid] Revert page size number | 'all' type to just number #5699

Merged
merged 12 commits into from
Mar 8, 2022

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Mar 8, 2022

Summary

closes #5697

Unfortunately the | 'all' typing (see #5547, particularly the first 4 commits I added) is simply causing too many downstream Typescript shenanigans in Kibana, so in this case typescript should trump API clarity for expedience 😢

  • Original PR/reversion had 0 representing all instead of -1, does -1 feel better?
  • Note: I actually don't recommend following along by commit on this PR, the git reverts caused a decent amount of noise and I slightly regret not just working off of main

QA staging

Checklist

  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen requested a review from thompsongl March 8, 2022 16:32
@cee-chen
Copy link
Member Author

cee-chen commented Mar 8, 2022

Sorry for force push noise, still going through the revert diff and cleaning up various changes we did want in main. Will comment again when done

@cee-chen cee-chen force-pushed the table-pagination-all branch 4 times, most recently from abc625d to 85f488f Compare March 8, 2022 17:24
0 is falsey and thus is failing a bunch of checks/fallthroughs, switching to `??` and `!=` null generally solves the problem
@cee-chen
Copy link
Member Author

cee-chen commented Mar 8, 2022

@thompsongl Alrighty, apologies for the noise, this should be all working and the diff looks more sane to me now 🙈

cc @cchaos on this as well, for once the staging link goes up - please feel free to ping if you think the examples are too confusing, particularly the EuiTablePagination one. I actually think the Math.ceil() logic getting simplified is better than before.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5699/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks, Constance!

I confirmed that the new eui.d.ts will resolve the type errors in Kibana.

@cee-chen cee-chen merged commit 0432bfd into elastic:main Mar 8, 2022
@cee-chen cee-chen deleted the table-pagination-all branch March 8, 2022 19:21
cee-chen added a commit that referenced this pull request Mar 8, 2022
- Missed this just as I was hitting merge on #5699, does not affect any source code
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5699/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants