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

Upgrade to EUI 1.1.0 #20411

Merged
merged 5 commits into from
Jul 6, 2018
Merged

Upgrade to EUI 1.1.0 #20411

merged 5 commits into from
Jul 6, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 3, 2018

No description provided.

@nreese
Copy link
Contributor Author

nreese commented Jul 3, 2018

blocked by elastic/eui#965

EuiPopover does not get displayed in an EuiFlyout. This occurs with the inspector

@nreese nreese added the blocked label Jul 3, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese changed the title Upgrade to EUI 1.0.1 Upgrade to EUI 1.1.0 Jul 5, 2018
@nreese nreese removed the blocked label Jul 5, 2018
@nreese
Copy link
Contributor Author

nreese commented Jul 5, 2018

elastic/eui#965 has been resolved in EUI 1.1.0 and now the inspector popover is visible.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese requested review from timroes and cjcenizal July 5, 2018 22:20
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Code LGTM, just had a few nitpicky suggestions. Didn't test locally.


test('add control btn', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: can we rename btn to button while we're here?

@@ -364,7 +364,10 @@ export function VisualizePageProvider({ getService, getPageObjects }) {
async setInspectorTablePageSize(size) {
const panel = await testSubjects.find('inspectorPanel');
await find.clickByButtonText('Rows per page: 20', panel);
await find.clickByButtonText(`${size} rows`, panel);
// Table size buttons are in popover element that is not under the inspectorPanel
// but are in an it's own portal attached directly to the body
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rephrase this? I didn't know what "Table size buttons" meant until I figured out the context -- at first I thought it was a reference to buttons that are table-sized.

// The buttons for setting table page size are in a popover element. This popover
// element appears as if it's part of the inspectorPanel but it's really attached
// to the body element by a portal.

await find.clickByButtonText(`${size} rows`, panel);
// Table size buttons are in popover element that is not under the inspectorPanel
// but are in an it's own portal attached directly to the body
const tableSizesPopover = await find.byCssSelector('.euiPanel');
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we can use a data-test-subj selector here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The buttons are created with EuiBasicTable and I do not think there is any way to attach data-test-subjs to the buttons

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

LGTM

@nreese nreese merged commit 61d7b91 into elastic:master Jul 6, 2018
nreese added a commit to nreese/kibana that referenced this pull request Jul 6, 2018
* Upgrade to EUI 1.0.1

* upgraded to eui 1.1.0

* fix setInspectorTablePageSize function in functional tests

* better comment about basic table pagination buttons
nreese added a commit that referenced this pull request Jul 6, 2018
* Upgrade to EUI 1.0.1

* upgraded to eui 1.1.0

* fix setInspectorTablePageSize function in functional tests

* better comment about basic table pagination buttons
@nreese nreese deleted the eui_1.0.1 branch July 13, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants