[CCI] Add functionality to OuiBottomBar to have the same width as the container element #708
Conversation
…iner element (opensearch-project#707) Co-authored-by: Andrey Myssak <[email protected]> Signed-off-by: Sergey Myssak <[email protected]>
SergeyMyssak
requested review from
seanneumann,
AMoo-Miki,
rednaksi91,
KrooshalUX,
BSFishy and
bandinib-amzn
as code owners
SergeyMyssak
changed the title
There was a problem hiding this comment.
It looks like this implementation probably fixes the issue, however it introduces a breaking change. What that means is that Dashboards would need to introduce a breaking change (3.0) to update to OUI 2.0, which is not on the roadmap or planned for anytime soon. Ultimately, that means that Dashboards wouldn't be able to consume the fix to the issue, at which point, why are we doing it in the first place.
The fix for the issue needs to be implemented in a backwards-compatible way, such that we only need to cut a minor version bump so that Dashboards is able to pick up the changes.
All in all, I think an approach where we use a resize observer on a specific element ID has a very ugly developer experience, and somewhat breaks React's "componentized" model. I added a few more details in the original issue.
I think in its current state, this PR shouldn't be merged until these concerns are taken care of.
| export const useResizeObserver = ( | ||
| container: Element | null, | ||
| dimension?: 'width' | 'height' | ||
| ) => { | ||
| const [size, _setSize] = useState({ width: 0, height: 0 }); | ||
| export interface UseResizeObserverProps { | ||
| element?: HTMLElement | null; | ||
| elementRef?: MutableRefObject<any> | RefObject<any> | null; | ||
| elementId?: string; | ||
| observableDimension?: 'all' | 'width' | 'height'; | ||
| shouldObserve?: boolean; | ||
| } | ||
|
|
||
| export const useResizeObserver = ({ | ||
| element, | ||
| elementRef, | ||
| elementId, | ||
| observableDimension = 'all', | ||
| shouldObserve = true, | ||
| }: UseResizeObserverProps) => { |
There was a problem hiding this comment.
Changing the prototype like this is a breaking change
There was a problem hiding this comment.
Yes, I know that this is a breaking change. At the moment I can see two ways around this, as only the interface of this function has changed. The first is to create a new hook, the second is to choose how it is processed depending on the container prop type. Is there another way to around this?
|
Closing this in favour of opensearch-project/OpenSearch-Dashboards#3978 |
Description
Untitled.mov
useResizeObserverwhen the hook failed to immediately detect changes and consequently did not promptly return the intended result.Issues Resolved
#707
Check List
yarn lintyarn test-unitBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.