[Observability Onboarding] Add customizable header for quickstart flows

[justinkambic]

Summary

Resolves #186397.

Adds a new customizable header for onboarding quickstart flows.

To add a new header, one must specify a logo type or EuiIconType, along with headlineCopy and descriptionCopy, and put a new entry in the headerContent record, and a corresponding case in the customHeaderProps helper function I have defined. When routes that require custom headers are defined, the function will pick up the necessary props and return them to the HeaderSection component where they're used to render a CustomHeaderSection. Otherwise, the component will render the default header.

If the consuming code specifies a SupportedLogo type, it's rendered as an EuiIcon. Otherwise, if an EuiIconType is specified, it renders as an avatar, per the design.

Code is WIP.

Adds new headers for quickstart flows

Kubernetes

OpenTelemetry

Autodetect

[obltmachine]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
• run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

[elasticmachine]

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

[flash1293]

Just tested and this doesn't work when answering the first question, then clicking the tile - I think it's because it's checking the path for kubernetes, not kubernetes/?category=infra.

In general, it's not explicitly stated in the issue (sorry for that), but by my understanding the idea was to have that header for all the flows, is that right @sileschristian ?

[justinkambic]

this doesn't work when answering the first question, then clicking the tile

Sorry I overlooked this. I should've marked this as a draft, I am adding some unit tests that have not been pushed yet. I was hoping to have that available by now but I am doing SDH for infra services this week so I am delayed on everything else I was doing. Thanks for uncovering the issue, I will address it and re-ping when it's ready.

[sileschristian]

In general, it's not explicitly stated in the issue (sorry for that), but by my understanding the idea was to have that header for all the flows, is that right @sileschristian ?

Yes. To be consistent that was the idea. Since we were working on the K8s, I opened just an issue for this one. But this should be for the rest (Otel, Firehose, Auto-detect, etc)

[justinkambic]

But this should be for the rest (Otel, Firehose, Auto-detect, etc)

Ok, let's merge this as a first step and then I will follow up and refactor this header to be general. Do we have copy for the other flows? Or should it keep the generic description text and just change the "Setting up {FLOW}", and have the logo update for whichever flow is covered?

[elasticmachine]

⏳ Build in-progress

• Buildkite Build
• Commit: 5f45981
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-188340-5f459815cadf
• Observability Deployment

History

• 💛 Build #222447 was flaky e37f192417f9effc808bff76857785bbc6babd31

cc @justinkambic

[thomheymann]

Very nice, this looks so much better than the previous design and makes it clearer which Quickstart flow the user is on.

Couple comments regarding the implementation below

[thomheymann]

What's the purpose of this switch statement? It looks like they're all the same values.

[justinkambic]

Essentially just making TypeScript happy because you cannot provide an EuiIconProps to EuiAvatarProps size as they have different values defined.

Basically, we want to accept xxl as a size for icons, but EuiAvatar doesn't support this size, so in cases where we want the avatar we can only accept xl. Maybe better to avoid supplying the EUI types, or inverting the rendering by accepting an avatar from the parent.

[thomheymann]

Wouldn't it be easier to just restrict the type of your LogoIcon components' size prop accordingly instead of trying to convert between the two?

If it's important that the prop works for both components (EuiIcon and EuiAvatar) you can use the intersection: EuiIconProps['size'] & EuiAvatarProps['size'].

If the prop is only used by EuiAvatar then simply use EuiAvatarProps['size']

[justinkambic]

Well, the parent component accepts a size prop that was previously only EuiIconProps['size'], but now we need to provide that to the Avatar component, whose size is a subset. I think you're right we should accept an intersection and we can use a type guard to infer the right type in the parent.

[justinkambic]

Let me know your thoughts on db27dde and if you think we can further clean it up.

[thomheymann]

Looks great, much simpler!

[thomheymann]

I think this is overcomplicating things a bit and I'm not sure if managing these icons and descriptions in a central location is desirable.

Is it not possible to restructure the render tree so that it reflects the designs?

Each Quickstart flow should define its own title, description and icon.

So instead of what we currently have:

  <PageHeaderThatNeedsToKnowAboutAllPossibleRoutes />
  <Route path="/auto-detect">
    <AutoDetectPanel />
  </Route>
  <Route path="/kubernetes">
    <SystemLogsPanel />
  </Route>
  <Route>
    <OnboardingFlowForm />
  </Route>

We can simply let each page render its own header:

  <Route path="/auto-detect">
    <CustomHeader />
    <AutoDetectPanel />
  </Route>
  <Route path="/kubernetes">
    <CustomHeader />
    <SystemLogsPanel />
  </Route>
  <Route>
    <PageHeader />
    <OnboardingFlowForm />
  </Route>

[justinkambic]

Yes this is acceptable as well. I was trying to make the changes backward compatible with the existing design but I am fine with re-implementing it in this way.

[justinkambic]

I explored this solution and the code is much cleaner:

        <EuiPageTemplate.Section paddingSize="xl" color="subdued" restrictWidth>
          <Routes>
            <Route path="/auto-detect">
              <AutoDetectHeader />
              <AutoDetectPanel />
            </Route>
            <Route path="/systemLogs">
              <Header />
              <BackButton />
              <SystemLogsPanel />
            </Route>
            <Route path="/customLogs">
              <Header />
              <BackButton />
              <CustomLogsPanel />
            </Route>
            <Route path="/kubernetes">
              <KubernetesHeader />
              <KubernetesPanel />
            </Route>
            <Route path="/otel-logs">
              <OtelHeader />
              <OtelLogsPanel />
            </Route>
            <Route>
              <Header />
              <OnboardingFlowForm />
            </Route>
          </Routes>
          <EuiSpacer size="xl" />
        </EuiPageTemplate.Section>

Unfortunately, nesting the header section inside the route kind of messes up a lot of the page layout:

EDIT: having thought about this some more, we could make a top-level page component that we provide with children within the routes if we wanted, and allow it to receive custom headers like I've shown above. This would help us avoid having to repeat the page template code for each route.

[thomheymann]

I don't think we need to hack any CSS, you are seeing these issues because your routes are already inside the EuiPageTemplate.Section when they should sit a level above.

In the new designs there are two different layouts. The first layout is the landing page with the background image and the second one is the layout for the quickstart flows with the icon and back button.

I would first create a component for both of these layouts which utilise the EuiPageTemplate components. Then each page can use whichever layout it needs using its own entry point:

App

<Routes>
  <Route path="/auto-detect">
    <AutoDetectPage />
  </Route>
  <Route path="/kubernetes">
    <KubernetesPage />
  </Route>
  <Route>
    <LandingPage />
  </Route>
</Routes>

KubernetesPage / AutoDetectPage

These entry points are optional, if you don't want to create a separate component for each page this markup could also sit directly in the App component inside each route. The point is that we have a reusable component for the different layouts in the designs that hides away the complexity / custom CSS.

<QuickstartPageLayout icon="kubernetes" title="Kubernetes">
  Kubernetes steps
</QuickstartPageLayout>

QuickstartPageLayout

<EuiPageTemplate>
  <EuiPageTemplate.Section>
    Header
  </EuiPageTemplate.Section>
  <EuiPageTemplate.Section>
    <BackButton />
    {children}
  </EuiPageTemplate.Section>
</EuiPageTemplate>

[justinkambic]

I don't think we need to hack any CSS, you are seeing these issues because your routes are already inside the EuiPageTemplate.Section when they should sit a level above.

Yes you're right of course, sorry. I was over my EoD and in a rush and when this didn't work right away I quickly posted a comment to keep the conversation going. I came back later with an edit that is very much in line with the solution you proposed here.

I'll ping again when I've implemented this and I agree it'll be much cleaner. Thanks for all the feedback!

[justinkambic]

@thomheymann think we are ok for another look here.

[mykolaharmash]

Looks great ✨ Added a few discussion points, but nothing major.

Also, not for this change, but we need to remove the second OTel header above the stepper (created an issue)

[mykolaharmash]

Appreciate tests 🙏 We've gotten out of habit writing them in onboarding haha. One thing though, I think "should render ..." kind of tests fit better with Storybooks. It takes care of the rendering part automatically plus we get a visual playground to test a component with different props. I don't mind keeping it this way, but feel free to change if you think Storybooks would indeed work better.

[justinkambic]

Do we have a storybook flow set up already? If so I'm happy to port, but I don't want to set storybook up as part of this patch. Agree it would be nice-to-have!

[mykolaharmash]

ah, I thought storybooks are setup globaly for all of Kibana, but you might be right that we don't have them for onboarding 😔

[justinkambic]

We should add them though, would be a nice dev quality of life enhancement.

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 0390664
• Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-188340-0390664f8c3a
• Observability Deployment

Failed CI Steps

• FTR Configs #86

Test Failures

• [job] [logs] FTR Configs #86 / task_manager with mget task claimer migrations 8.5.0 migrates active tasks to set enabled to true

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
observabilityOnboarding	225	235	+10

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
observabilityOnboarding	243.7KB	246.5KB	+2.8KB

History

• 💚 Build #225154 succeeded 53669bb
• 💔 Build #224786 failed db27dde
• 💔 Build #224695 failed c9da466
• 💔 Build #224097 failed a13c72b8b34c5c21aca52d9c8e3daf2874628246

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @justinkambic

[mykolaharmash]

Looks great, thank you @justinkambic!