-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Observability Onboarding] Add customizable header for quickstart flows #188340
base: main
Are you sure you want to change the base?
[Observability Onboarding] Add customizable header for quickstart flows #188340
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
d129791
to
fbcb8ba
Compare
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
4b0a833
to
1505c58
Compare
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 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 ? |
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. |
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) |
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? |
e37f192
to
5f45981
Compare
⏳ Build in-progress
History
|
Hi @justinkambic , the header is way too high for me, is there something missing? ![]() Also @sileschristian confirmed this should be done for all flows, are you planning to roll it out to the others on separate PRs? Mostly asking as it might make things easier to not have a special case for it. |
This is quite strange, I have screenshots I saved while developing this that do not show the header doing this, but now I am seeing it as well. I will fix it 👍
My plan was to go ahead and merge this and then do a separate refactor to update it. I am fine with doing all of the work on this branch though, if that's preferred. As I understand it, we need a separate codepath for this header regardless as we are overriding the existing default header for all of these cases. The way the app is structured, the header was previously independent of the routes and thus we need to override it for routes like |
3a9ee5f
to
3a0c019
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
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 withheadlineCopy
anddescriptionCopy
, and put a new entry in theheaderContent
record, and a corresponding case in thecustomHeaderProps
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 theHeaderSection
component where they're used to render aCustomHeaderSection
. Otherwise, the component will render the default header.If the consuming code specifies a
SupportedLogo
type, it's rendered as anEuiIcon
. Otherwise, if anEuiIconType
is specified, it renders as an avatar, per the design.Code is WIP.Adds new headers for quickstart flows
Kubernetes
OpenTelemetry
Autodetect