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

EUI dashboard add panel #17374

Merged
merged 25 commits into from
May 24, 2018
Merged

EUI dashboard add panel #17374

merged 25 commits into from
May 24, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Mar 23, 2018

screen shot 2018-05-17 at 1 39 12 pm

@nreese nreese added the Feature:Dashboard Dashboard related features label Mar 23, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor

Wow, how are you so productive @nreese??

Screenshot looks great, but I notice there isn't a type column anymore. Can it be easily added back in? Wonder if we'll need a bit more horizontal room. Is it easy to add more horizontal room to these side panel pop outs? I could see more columns eventually being useful, like last updated time.

@trevan
Copy link
Contributor

trevan commented Mar 27, 2018

Should the "add" button at the top be highlighted in some manner to indicate that the panel is connected to it?

@nreese
Copy link
Contributor Author

nreese commented May 17, 2018

Screenshot looks great, but I notice there isn't a type column anymore

@stacey-gammon What type column? The current table does not display any visualization type data.

screen shot 2018-05-17 at 1 02 37 pm

@nreese
Copy link
Contributor Author

nreese commented May 17, 2018

Should the "add" button at the top be highlighted in some manner to indicate that the panel is connected to it?

@trevan I am going to change the styling so the menu takes up the entire height of the screen and flyout owns the focus. This way, it will not be possible to click any of the other nav buttons and the flyout will close if the user clicks outside of the flyout.

@nreese
Copy link
Contributor Author

nreese commented May 17, 2018

Is it easy to add more horizontal room to these side panel pop outs

@stacey-gammon Right now the CSS style makes the flyout be 33% of page width but that can easily be increased. What do you think makes the most sense?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor

Ah, my badd @nreese - didn't realize the original add panel table dropped the type column - I was thinking of the visualize landing page. In that case, it looks good the way you have it! Good to know we can easily increase width if we want to add more columns later.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

👏

@@ -0,0 +1,161 @@
import _ from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, but since multiple places use the saved object finder, I'm thinking we should put this in a more generic spot than in dashboard. Maybe ui/saved_objects? The angular one is in a mix of ui/partials and ui/directives which I think is a bad spot for it (and obvi wouldn't fit well with the react version).

Then other teams can use it when they start reactifying too.

}

SavedObjectFinder.propTypes = {
addNewButton: PropTypes.node,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make this usable on Discover too (the Open panel uses the saved object finder), it's not an add new button but a manage button, so we might just want to call it 'callToActionButton' or something more generic.

isOpen = false;
};
const find = (type, search, page, perPage) => {
return savedObjectsClient.find({
Copy link
Contributor

Choose a reason for hiding this comment

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

Original saved object finder has logic for filtering out lab visualizations based on the advanced setting, think we need to copy that here.

Can you add a test for this too?

Copy link
Contributor Author

@nreese nreese May 22, 2018

Choose a reason for hiding this comment

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

Should the filtering out labs visualizations logic exist in show_add_panel or saved_object_finder?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the saved object finder takes in type and find, I think it kinda fits best in show_add_panel. Then we don't have to add more (if type == visualize { do this} ) logic inside of the saved object finder which tries to be more generic.

Eventually this add panel stuff will change too so it fits with the more generic embeddable concept, maybe once we get closer to k7 and we have a design that doesn't include the two tabs. Then embeddables can have a generic isLab parameter and we don't need the specific type checks.

@@ -132,7 +154,7 @@ export function DashboardAddPanelProvider({ getService, getPageObjects }) {
log.debug(`DashboardAddPanel.addVisualization(${vizName})`);
await this.ensureAddPanelIsShowing();
await this.filterEmbeddableNames(`"${vizName.replace('-', ' ')}"`);
await find.clickByPartialLinkText(vizName);
await testSubjects.click(`addPanel${vizName.split(' ').join('-')}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you added this in, do you still need to have the logic that changes all the spaces to dashes in the callers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here. Could you explain a little more?

Copy link
Contributor

Choose a reason for hiding this comment

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

You changed some code above, turning things like
await dashboardAddPanel.addVisualization('Rendering Test: animal sounds pie'); into await dashboardAddPanel.addVisualization('Rendering-Test:-animal-sounds-pie'); Which makes it seems like addVisualization should no longer be called with a visualization name with spaces in it, but here you are taking a name with spaces and dynamically adding the dashes. So seems like the other changes could be discarded.

await testSubjects.click('paginateNext');
await PageObjects.header.waitUntilLoadingHasFinished();
// Clear all toasts that could hide pagination controls
const toasts = await find.allByCssSelector('.euiToast');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will be very useful logic for other tests too. Maybe put into PageObjects.header? or PageObjects.common? Or add a new toastsService?

@@ -36,8 +36,6 @@ export default function ({ getService, getPageObjects }) {
const panelCount = await PageObjects.dashboard.getPanelCount();
expect(panelCount).to.be(27);

await PageObjects.dashboard.waitForRenderComplete();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little worried why leaving this in broke the tests. Reporting relies on render complete so if something isn't working right, it might be a bug. Was it just flaky or consistently broken?

Although it's in a weird spot anyway and kinda looks like it was accidentally left in (I'm sure my bad). So I'm fine with removing it, just still curious how it caused things to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would run locally fine with that line in. It always failed on CI if that line is in. One panel never set render to true and I could not figure out why - and why it would work locally

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@stacey-gammon stacey-gammon 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 9c3481c into elastic:master May 24, 2018
nreese added a commit to nreese/kibana that referenced this pull request May 24, 2018
* EUI add panel

* implement add functionallity

* style flyout so top nav is not covered

* add noItemsMessage

* add new visualization button

* remove angular add_panel template

* seperate search bar and table into its own component

* fix functional tests

* make slide out panel own focus to avoid weirdness of letting other buttons in nav from getting clicked and leaving slide out open

* remove deprecated method componentWillMount

* add jest test for DashboardAddPanel

* fix paging and replace EuiSearcBar with EuiFieldSearch

* fix functional tests

* fix dashboard filter bar functional test

* another functional test fix

* add more context to functional test failure message

* give search input a default value

* remove call to waitForRenderComplete to see if tests will pass

* fix dashboard filtering test

* updates from Stacey-Gammon review

* support filtering out lab visualizations

* add functional test for testing visualize:enableLabs with add panel

* add sorting by title to SavedObjectFinder componenet

* move add panel tabs to state

* clean up labs test differently
nreese added a commit that referenced this pull request May 24, 2018
* EUI add panel

* implement add functionallity

* style flyout so top nav is not covered

* add noItemsMessage

* add new visualization button

* remove angular add_panel template

* seperate search bar and table into its own component

* fix functional tests

* make slide out panel own focus to avoid weirdness of letting other buttons in nav from getting clicked and leaving slide out open

* remove deprecated method componentWillMount

* add jest test for DashboardAddPanel

* fix paging and replace EuiSearcBar with EuiFieldSearch

* fix functional tests

* fix dashboard filter bar functional test

* another functional test fix

* add more context to functional test failure message

* give search input a default value

* remove call to waitForRenderComplete to see if tests will pass

* fix dashboard filtering test

* updates from Stacey-Gammon review

* support filtering out lab visualizations

* add functional test for testing visualize:enableLabs with add panel

* add sorting by title to SavedObjectFinder componenet

* move add panel tabs to state

* clean up labs test differently
@nreese nreese deleted the euiAddPanel branch July 18, 2018 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features v6.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants