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

Solution nav not rendering when KibanaPageTemplate has no data #143114

Closed
alisonelizabeth opened this issue Oct 11, 2022 · 15 comments
Closed

Solution nav not rendering when KibanaPageTemplate has no data #143114

alisonelizabeth opened this issue Oct 11, 2022 · 15 comments
Labels
bug Fixes for quality problems that affect the customer experience Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@alisonelizabeth
Copy link
Contributor

Kibana version:
main (Confirmed it is working as expected on 8.5)

Describe the bug:
The side solution nav does not render when the KibanaPageTemplate has no data.

Steps to reproduce:
I was able to reproduce this for the Observability apps. I'm not sure if it's only limited to that or not though.

  1. Navigate to /app/home#/getting_started
  2. Select "Monitor my environments"
  3. Start the tour.
  4. Click on one of the elements with the solution nav, for example "Metrics Explorer".
  5. Note that the solution nav disappears when the empty prompt displays.

Expected behavior:
The solution nav should render when the user does not have any data.

Screenshots (if relevant):
Current behavior:
Screen Shot 2022-10-11 at 10 15 11 AM

Expected behavior:
Screen Shot 2022-10-11 at 10 16 05 AM

Any additional context:
Here is the code that handles the Observability page template rendering. I see there were some EUI changes made in #139524, however, assuming the labels are correct, this was released in 8.5.

This bug is problematic specifically for the guided onboarding project, as the existing EuiTour expects elements to exist in the solution nav.

@alisonelizabeth alisonelizabeth added bug Fixes for quality problems that affect the customer experience Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Oct 11, 2022
@alisonelizabeth
Copy link
Contributor Author

This looks like it might be a regression caused by #141043

@majagrubic
Copy link
Contributor

majagrubic commented Oct 12, 2022

I believe this is the PR that caused regression:
98f73d6#diff-28b5ec37e588525c45f1457c4dcb26d1234676d02f4a250341eed497744be405R34

Specifically this change to no_data_config_page:
https://github.com/elastic/kibana/blob/main/packages/shared-ux/page/no_data_config/impl/src/no_data_config_page.tsx#L55

With this change, the sidebar is rendered at the incorrect place in the DOM.

When I changed the code locally to EuiPageTemplate_Deprecated, which takes sidebar as its own prop, it works fine. But I would like to avoid doing that, as I see no point in reverting to a deprecated component. @constancecchen do you have any better ideas here?

@majagrubic
Copy link
Contributor

majagrubic commented Oct 14, 2022

Just to provide some additional context here. This is the DOM structure before the change - both solutionNav and euiPageTemplateInner (which holds NoDataPage) are nested under page template:
Screenshot 2022-10-13 at 18 57 59

With the new DOM structure EuiPageTemplateInner is now holding solutionNav, and they are both nested under noDataPage:
Screenshot 2022-10-13 at 18 59 25

EuiPageTemplateInner has this flex-direction: column property, which causes this to render improperly:
Screenshot 2022-10-13 at 19 00 37

@majagrubic
Copy link
Contributor

majagrubic commented Oct 14, 2022

My PR linked above introduced this flex property on solution nav:
https://github.com/elastic/kibana/blob/main/packages/shared-ux/page/solution_nav/src/with_solution_nav.styles.ts#L14

The flex property was previously coming from some eui class and was not getting applied with the new change to emotion. Without it, the content next to side nav is not expanded/collapsed properly, as visible below:
drawer

This works as intended on all other Solution pages besides Observability.

@1Copenut
Copy link
Contributor

I looked into this issue yesterday and think I found a root cause. The EUI Page Template Sidebar docs mention

Sidebars must be direct children declared in the same component.

In order for the template configurations to properly account for the existence of a sidebar, it needs to clone the element which can only be performed on direct children.

I refactored the example code to import the sidebar code instead of typing it out as a sibling element to the section code. This caused a same behavior as the original issue described. The screenshots below illustrate what I did to recreate the issue.

My hunch is moving the sidebar or section code into one file instead of using an import statement should fix the issue without additional work in EUI. Let me know if that doesn't fix the issue and we'll take another look


Screen Shot 2022-10-13 at 4 54 58 PM


Screen Shot 2022-10-13 at 4 55 17 PM


Screen Shot 2022-10-13 at 4 54 06 PM

@majagrubic
Copy link
Contributor

@1Copenut thank you for looking into this. putting this in the same side makes it definitely look better, but the culprit I see is still this direction: column on page template inner. Any way to get rid of it?

Before:
direction_column

With direction: column commented out:
direction-row

@majagrubic
Copy link
Contributor

majagrubic commented Oct 14, 2022

Also, the top is not set properly for the side nav; that is a result of my change. I still don't understand what makes Observability Page Template different than the one in all the other solutions. I will have a look into that.

@1Copenut
Copy link
Contributor

I wish I had a better answer than I'm not sure. I read through the docs a number of times and didn't find any indication why the Observability Page Template is handling the left nav different than the others. I've let Chandler know about this issue so he can take a fresh look next week.

@chandlerprall
Copy link
Contributor

I've asked @miukimiu to look into this next week

@miukimiu
Copy link
Contributor

miukimiu commented Oct 21, 2022

I looked at this. And these are my findings.

The issue only happens if we pass a css prop to <EuiPageTemplate.Sidebar />. So if we pass the styles in the style prop the sidebar goes to the right place. Even an empty css prop creates the issue.

The way the EuiPageTemplate works is to check all the children and if the child.type is a EuiPageTemplate.Sidebar it goes below EuiPageOuter if not it goes to bellow EuiPageInner:

React.Children.toArray(children).forEach((child, index) => {
    if (!React.isValidElement(child)) return; // Skip non-components

    switch (child.type) {
      case EuiPageSidebar:

      default:

It seems that in Kibana every time we pass a css prop to any EUI component the children type gets wrong. So it assumes is not a EuiPageSidebar and that's why it renders inside the EuiPageInner:

// console.log(children) that is a <EuiSidebar css={css``}  />
type: {$$typeof: Symbol(react.forward_ref), render: ƒ}

// console.log(children) that is a <EuiSidebar  /> no CSS prop
type: ƒ EuiPageSidebar(_ref)

I tested in CodeSandbox (also locally) and if I pass a css prop to EuiPageTemplate.Sidebar it works well: https://codesandbox.io/s/withered-dream-q3svfh?file=/demo.tsx. It works because the children that is a sidebar get the type EuiPageSidebar.

So I'm not sure why this is happening in Kibana.

How to fix

One solution could be avoid the css prop and pass these styles in the style prop.

But IMO we should find out why any EUI component that has a css prop in kibana is not getting the right type. Why does passing the css prop transform the component?

So this is something a couldn't find answer. @chandlerprall @majagrubic could this be the way emotion is configured in kibana (babel for example)?

@cee-chen
Copy link
Member

I plan on a taking a look at this tomorrow to investigate a potential fix on EUI's side. Amazing job debugging the underlying css issue @miukimiu - my best guess is that it's due to Emotion creating wrappers around components (visible in React Dev Tools). I'm hoping we can create more permissive/intelligent logic around the child.type to dive into Emotion wrapped components and detect their true type.

@cee-chen
Copy link
Member

@majagrubic elastic/eui#6324 - I have a fix in but it likely won't land in Kibana main for another week or two. In the interim I would suggest using @emotion/css instead of @emotion/react to generate a className, and pass it in as a className prop.

@majagrubic
Copy link
Contributor

I merged a workaround as suggested by @constancecchen. This should now fix Observability problem

@alisonelizabeth
Copy link
Contributor Author

Thank you @majagrubic and @constancecchen! I'm going to close this issue.

@cee-chen
Copy link
Member

cee-chen commented Nov 2, 2022

FYI @majagrubic that the css fix for EuisideBar is in #141279 and will hopefully be merged by EOW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

No branches or pull requests

6 participants