highlight sample data section for new users

[nreese]

fixes #20742

Highlights add data footers for new kibana instance. A new kibana instance is one with no index-pattern saved objects

cc @alexfrancoeur

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[chrisdavies]

LGTM. Pulled and ran. Works as expected.

[chrisdavies]

Just wanted to double-check that EUI doesn't already have a generic / reusable style for these kinds of highlights.

[chrisdavies]

I think this is a case where promises are a good bit cleaner. I think this entire block could be rewritten like this, and would be easier to grok:

this.setState({ isLoading: true });

this.props.find({
  type: 'index-pattern',
  fields: ['title'],
  search: `*`,
  search_fields: ['title'],
  perPage: 1
})
  .catch(() => ({
    // Ignore errors, as this is just a cosmetic detail
    total: 0
  }))
  .then(({ total }) => this._isMounted && this.setState({ isLoading: false, isNewKibanaInstance: total === 0 }))

[stacey-gammon]

Have not pulled down the code yet to verify but it seems like the loading time of the index patterns find call will block the entire home app from rendering because of this check.

Perhaps a better way to solve this that would eliminate the need for async handling in this react component is to pass in a boolean isNewKibanaInstance as a prop, then do the async fetch call higher up.

I don't think we want to block the rendering of the page on that call, so we should probably default the boolean to false (renders without the callout). Once the call returns, if it's true, it'll highlight that section as blue.

Could also solve this by not blocking this on isLoading but I think the less components we have dealing with asynchronicity the better. Makes testing easier too, less mocking. Could even push this up to the angular/controller level. We will probably need this value sooner anyhow, once we eventually do that whole page flash screen.

Thoughts?

[nreese]

it seems like the loading time of the index patterns find call will block the entire home app from rendering because of this check

Yes, that is on purpose. I wanted to avoid having screen flicker or the footer section flash when the async function returns. I blocked the entire render so everything can render at once. The call to find index patterns will always be extremely fast so this seemed like a fair trade-off.

[stacey-gammon]

The call to find index patterns will always be extremely fast

This is not necessarily so. Someone on a slow internet connection will see a delay. Someone with a bogged down elasticsearch cluster could see a delay. This is also the first page people see when booting up Kibana. It doesn't seem worth it to delay the entire page loading to maybe give something an extra highlight. I think it's much more important to load fast, especially for users where it's not their first time booting up Kibana - they don't care about the style of the sample data callout but may only want the recently accessed section to navigate to their last dashboard as soon as possible.

I feel like we might be losing out on one of the benefits of react by blocking the page render for one small component.

We usually use loading indicators in order to render a page as fast as possible when some components make async calls, though a loading indicator in this case doesn't make much sense. Maybe if it looks awkward we can use a css transition to make it smoother? Have you tried it this way and noticed a flicker? Maybe in most cases it won't even be noticeable because the call should be fast, most of the time?

Since this seems to be up for debate, perhaps we should just style it blue all the time and avoid the delay and complexity the conditional aspect adds to this last minute change. I think everyone was initially okay going that route. cc @alexfrancoeur

[alexfrancoeur]

@stacey-gammon @nreese The best experience (in the short amount of time we have) would be to conditionally color. But if we don't think this will be possible before 6.4 FF, we can style blue the entire time for 6.4. I really don't think we can leave this experience for long and will need to introduce #18828 as soon as we can.

[nreese]

I can update the logic to remove blocking rendering. I think the highlighting should be conditional so slight page redraw is better than nothing

[stacey-gammon]

I really don't think we can leave this experience for long and will need to introduce #18828 as soon as we can.

@alexfrancoeur, Looks like @nreese and I have come to a decision, so I will continue reviewing with the conditional aspect. This is an example though, of how seemingly very simple, last minute changes end up taking up valuable time and are usually not as straightforward as they sound.

I understand that you want us to add the splash page as soon as we can but we have a lot of other things that also need to happen asap too (phantom to chromium, alerting, fix blocker bugs, get flaky tests under control).

It feels like increasingly we have too many high priority items.

[stacey-gammon]

Just to try to explain my frustration a little more. Yesterday I was in a couple hours of meetings, had to dig into a few SDH tickets about reporting, zoomed with some folks to try to solve some of the flaky test issues, reviewed this PR. Today I've been doing the same. Now this PR which changes the highlight of a little block of code will probably get into 6.4 because it's very important but the PR that fixes the chromium bug so that cloud could default to chromium might slip because I have half a day left before 6.4 FF, more meetings and notes to write up, and still have to finish reviewing this PR.

This is the trouble with having too many high priority items. It's difficult to intelligently make a decision on what to focus on and what to risk slipping. I would have rather had the chromium fix in than this color change.

I might be able to get the chromium bug into 6.4 because it's a "bug" so not restricted to the feature freeze dates, but there are also blockers that I need to focus on first, and I don't like using that excuse as an escape hatch to check code in post FF.

Hope I'm not coming off as aggressive, I just want to help everyone understand some of the difficulties the developers face, and why it may seem like important things don't get done (esp after 6.4 slipped by over a month). Also, I'm trying to not spend too much time writing this up, so I can get back to those other things 😸

[alexfrancoeur]

@stacey-gammon thank you for reviewing and I understand the frustration of last minute changes. This was not something that was originally planned for a number of reasons but can potentially have high impact on the getting started experience across the stack and in Cloud specifically. I know it's seems small, but conditionally styling the background of the sample data panel will draw the attention of net new users which results in a significantly better experience. This is an example where small changes can have huge impact and I really do apologize for bringing this to your attention so close to FF. I can't tell you how much I appreciate the work you and @nreese did to get this in.

In my last comment, I said "as soon as we can" with a full understanding of what's currently on this teams roadmap. If there is conditional styling, we may be able to live without the splash screen for a bit longer given the other priorities on this team. I know that after multiple conversations with design, this is by far the preferred approach. The original implementation of Kibana Home was actually an interstitial page as well. Though it was reverted, I wonder if any of the code is re-usable. #11805

[stacey-gammon]

lgtm, thanks for making the changes. Tested locally and reviewed code.

[nreese]

jenkins, test this

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request