opensearch-project / oui

OpenSearch UI Framework
Apache License 2.0
33 stars 69 forks source link

[CCI] Add new functionality to OuiBottomBar to have the same width as the container element #707

Open SergeyMyssak opened 1 year ago

SergeyMyssak commented 1 year ago

Is your feature request related to a problem? Please describe.

We need to introduce a new functionality to the OuiBottomBar, which will enable it to have the same width as the container element. The rationale behind this is due to a particular problem that we have encountered - https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3336 and request to consider different approaches - https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3804#issuecomment-1500751853.

The issue is that the location of the OuiBottomBar component may not have any knowledge about its container, including its width. Since the width of the container is not fixed and can vary due to external factors, it is necessary to adjust the OuiBottomBar component width accordingly. Using a predefined position for the OuiBottomBar component in container styles is not always practical because these values can change or be dynamic.

Describe the solution you'd like

In order to resolve this issue, it is necessary to create a fixed-position solution for the OuiBottomBar that can adapt to the width of its container.

BSFishy commented 1 year ago

One thing to note here is that React components usually don't, and moreover shouldn't, be able to access their containers. If a component has access to its container that breaks the "componentized" nature of React. You could resolve this by having a prop that describes how far from the edges of the screen the bottom bar should be, or in this specific case, maybe even just a boolean if the navigation bar is open. However, all in all, I think a component trying to get info about its container is a bad idea.

kgcreative commented 1 year ago

Honestly, this may just be a case of ensuring that bottom bar is in the right container, and making sure that the width is set to 100%. I suspect we might be able to do this via CSS + layout (Which may mean revising the way we recommend we lay out certain pages), rather than trying to use absolute positioning and other tricks

KrooshalUX commented 1 year ago

I think this is important context - https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3336#issuecomment-1429010591

Lets check the use of page template before we try to make changes to the bottom bar https://oui.opensearch.org/1.0/#/layout/page#showing-a-bottom-bar

There is displacement to also consider, to users can reach the true end of the page as well, so abs pos makes me nervous.

ashwin-pc commented 1 year ago

@kgcreative i agree, this is a matter of appending the bottom bar to the correct container. The current implementation for the bottom bar defaults to using a react portal to append it to the body tag by default. This takes it out of context from the sidenav and all other page elements. Unfortunately @KrooshalUX its not as simple as setting the postion to sticky since even then the context is nested too deep. i.e.

Screenshot 2023-04-14 at 4 31 23 PM

The simplest fix here would be to extent the usePortal prop to also allow the user to specify what the target DOM element for the portal is so that we can append the bottom bar to the right container. Then in OSD, we can default its usage to the app container instead of the body tag.

SergeyMyssak commented 1 year ago

Hello everyone! Thanks for the active discussion on this issue and for sharing your ideas. However, I think this issue is more tricky than it seems at first glance.

@BSFishy, thanks for your comment. I initially thought that "componentized" referred to breaking down a user interface into smaller, reusable pieces called "components". I believe we are not violating this principle because we are not changing the behaviour of the parent component in the child, and our component remains small and reusable. I understand that we should try to keep our components simple and follow React's principles. However, there may be situations where it is necessary to deviate slightly from them if the business demands it.

@BSFishy, regarding your first solution, I have already done something similar https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3804, but with class names. The thing is that we can't know in the component if the navigation bar is open, maybe we need to implement something new to get this information, but for now this solution doesn't fit much, and also I was asked to consider other ways than this simple one. As for the second solution, using a boolean to determine if the navigation bar is open, I don't think the bottom bar should be aware of the navigation bar. This solution creates an explicit and high coupling between two different components.

Regarding my proposed solution, I understand that it is generally not considered best practice for a child component to access the size of its parent component via ref and id, since we are deviating slightly from React's principles of not working directly with the DOM. In our case, using ResizeObserver to get the size of the parent component is a more or less appropriate approach, as it is a common way to get this information. However, we must be careful to avoid excessive updates, as resizing events can occur frequently and have a negative impact on performance. We also need to ensure that the parent component has a unique ID so that the child component can accurately retrieve the correct parent component.

@kgcreative, @ashwin-pc, thanks for your comments! As for the solution to this problem, where we need to change CSS styles and positioning with usePortal, I don't think that's enough. The main point is that we are using fixed positioning, not absolute. Fixed positioning is similar to absolute positioning, except that it does not respect any relative parents or ancestors, only the viewport. So changing its placement in the DOM will not solve this problem, so we need to look for another solution.

ashwin-pc commented 1 year ago

@SergeyMyssak yeah you are right about the fixed positioning issue. But the bottom bar component actually supports 3 position types. "fixed" "sticky" and "static". allowing to specify the portal node alone does not fix the issue, but that with the sticky position will. Since we already have the provision to set the position type, the only change we need to introduce in OUI is the portal change. Do you think that even this is not sufficient?

SergeyMyssak commented 1 year ago

Hello @ashwin-pc! I have tried to implement it as you suggested. Unfortunately, I have not been able to implement it with portal and sticky positioning, it may not even be possible. The point is that the sticky element switches between relative and fixed depending on the scrolling position. So we can't place this element at the bottom of the screen (viewport), it will be placed at the end of the parent component, whether we use portal or not. We also cannot use absolute positioning and inside sticky because it is positioned relative to the nearest positioned ancestor, which means it will also be positioned at the end of the parent component. The only solution I've found, using the portal and sticky positioning, is to place it at the top. This element will be in the flow DOM and will stick to the top when scrolling. I opened https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3949 with this solution

ashwin-pc commented 1 year ago

@SergeyMyssak you are right, sorry, i was making my recommendation assuming that we were already using OuiPageTemplate in our layouts. OSD layouts dont use the OUI styleguide recommendations today. Looks like we are using https://oui.opensearch.org/1.1/#/layout/page#legacy-layout instead of the recommended OuiPageTemplate. I think the right approach here would be to modify the rendering service to use Page Templates instead of the legacy layout so that we can follow the recommended OUI styleguide. Not sure what the effort here is to make that switch, but im happy to help you in exploring that change

SergeyMyssak commented 1 year ago

Hello @ashwin-pc, I did it the way you recommended, could you please check https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3978? Should I transfer the logic from createPortal to OUI BottomBar or should I concentrate on replacing the legacy layout with OuiPageTemplate?

ashwin-pc commented 1 year ago

Wow, great job on making it work without needing to update to using the PageTemplate. I still think that the createPortal solution you use in https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3978 should happen directly iin OUI, but for now that solution works. You can ignore updating the app to use PageTemplates for now :)

BSFishy commented 1 year ago

Great work! If there are no objections, I think this issue can be closed, as it seems its primary goal has been achieved. We can open follow-up issues (for example, updating the app to use PageTemplate) and deliberate more there

ashwin-pc commented 1 year ago

Id still like to use this issue to add/edit the Bottombar props to allow it to be portaled to any referenced DOM element to simplify following the page template pattern. Right now this is being done manually, but if we plan on making this pattern more common, this prop will be quite necessary.