-
Notifications
You must be signed in to change notification settings - Fork 72
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
[CCI] Add new functionality to OuiBottomBar to have the same width as the container element #707
Comments
…iner element (opensearch-project#707) Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
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. |
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 |
I think this is important context - opensearch-project/OpenSearch-Dashboards#3336 (comment) 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. |
@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 The simplest fix here would be to extent the |
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 opensearch-project/OpenSearch-Dashboards#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. |
@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? |
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 opensearch-project/OpenSearch-Dashboards#3949 with this solution |
@SergeyMyssak you are right, sorry, i was making my recommendation assuming that we were already using |
Hello @ashwin-pc, I did it the way you recommended, could you please check opensearch-project/OpenSearch-Dashboards#3978? Should I transfer the logic from |
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 opensearch-project/OpenSearch-Dashboards#3978 should happen directly iin OUI, but for now that solution works. You can ignore updating the app to use PageTemplates for now :) |
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 |
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. |
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
Signed-off-by: Sergey Myssak <[email protected]>
Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
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 - opensearch-project/OpenSearch-Dashboards#3336 and request to consider different approaches - opensearch-project/OpenSearch-Dashboards#3804 (comment).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 theOuiBottomBar
component width accordingly. Using a predefined position for theOuiBottomBar
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.The text was updated successfully, but these errors were encountered: