-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
De-angularize discover doc_table pager #40999
Conversation
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
Pinging @elastic/kibana-app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested and works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM! Just had some suggestions on ways to use EUI instead of KUI. Didn't test locally.
onPagePrevious: jest.fn(), | ||
}; | ||
const wrapper = mountWithIntl(<ToolBarPagerButtons {...props} />); | ||
wrapper.find('[data-test-subj="btnPrevPage"]').simulate('click'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, EUI publishes a helper that can make this a little terser:
import { findTestSubject } from '@elastic/eui/lib/test';
findTestSubject(wrapper, 'btnPrevPage').simulate('click');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good hint, I've adapted the test
|
||
export function ToolBarPagerButtons(props: Props) { | ||
return ( | ||
<div className="kuiButtonGroup"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is possible, but have you considered using the EuiPagination component instead? https://elastic.github.io/eui/#/navigation/pagination
It looks like it will substitute well from a UI-perspective, which would reduce the dependency upon the deprecated KUI library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider this in a future PR, my current target is to remove angular of discover, I'm aware there is more technical debt here, but I would remove it step by step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will definitly use the EuiPagination
after saying goodbye to angular. thank you for the hint
|
||
export function ToolBarPagerText({ startItem, endItem, totalItems }: Props) { | ||
return ( | ||
<div className="kuiToolBarText" data-test-subj="toolBarPagerText"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to use EuiText to format this text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, I will address this in the EUIFication of the whole doc_table
PR
💔 Build Failed |
99b2428
to
8bd0dcc
Compare
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
retest - so much unrelated Jenkins error today, should have merged it yesterday :) |
jenkins test this |
💚 Build Succeeded |
* Convert pager buttons to react * Convert pager text to react * Move pager factory to lib folder * Adapt directives to use react component * Add jest tests
Summary
Rewrite
doc_table
pager in ReactPart of #38646
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately