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.

Sign up for GitHub

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

Jump to bottom

[CCI] Allow to portale BottomBar to any referenced DOM element #758

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[CCI] Allow to portale BottomBar to any referenced DOM element #758

wants to merge 4 commits into from

Conversation

SergeyMyssak
Copy link
Collaborator

Description

Allow to portale BottomBar to any referenced DOM element

Issues Resolved

#707

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Commits are signed per the DCO using --signoff

By 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.

@BSFishy BSFishy added the CCI College Contributor Initiative label May 15, 2023
BSFishy
BSFishy reviewed May 15, 2023
View reviewed changes
Copy link
Contributor

@BSFishy BSFishy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this implementation! A few minor comments but overall looks really good

scripts/jest/config.json Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.tsx Outdated
Comment on lines 61 to 79
type _BottomBarExclusivePositions = ExclusiveUnion<
{ position?: 'static' | 'sticky' },
{
position?: 'fixed';
/**
* Whether to wrap in an OuiPortal which appends the component to the body element.
* Whether to wrap in OuiPortal. Can be configured using "insert" prop.
* Only works if `position` is `fixed`.
*/
usePortal?: boolean;
/**
* Whether the component should apply padding on the document body element to afford for its own displacement height.
* Only works if `usePortal` is true and `position` is `fixed`.
* Configuration for placing children in the DOM. By default, attaches children to the body element.
* Only works if `position` is `fixed` and `usePortal` is true.
*/
affordForDisplacement?: boolean;
},
{
insert?: OuiPortalInsert;
/**
* How to position the bottom bar against its parent.
* Whether the component should apply padding on the document body element to afford for its own displacement height.
* Only works if `position` is `fixed` and `usePortal` is true.
*/
position: 'static' | 'sticky';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand this correct, options are:

{
  position: 'static' | 'sticky'
}

or

{
  position: 'fixed',
  usePortal: ...,
  insert: ...,
  affordForDisplacement: ...,
}

i.e. just adding insert to position: fixed? Are there any combinations that worked previously but don't with this change?

Copy link
Collaborator Author

@SergeyMyssak SergeyMyssak May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I wrote this a little wrong, I've rewritten this interface and I'm afraid it's going to be a breaking change, because usePortal was true by default, and now it's false. It used to apply only to fixed position, but now it can apply to all positions, so if we leave it true, we will have not expected behavior for others by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that given the same code before and after this change, if the functionality remains consistent, we shouldn't consider it breaking. I.e. if I don't specify usePortal before and after this change, it should look and function the same, and that should be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. The point is that we had usePortal and affordForDisplacement as defaults true and now they are false, this change leads to different behavior if these properties were not explicitly specified. This PR in itself is breaking because we allow BottomBar of any position type to be portable, which means that the behavior that was the default for fixed position should apply to other types, but in order to avoid not-expected behavior when using BottomBar, we need to remove some defaults and specify them ourselves if needed

src/components/portal/portal.test.tsx Outdated Show resolved Hide resolved
src/components/portal/portal.tsx Show resolved Hide resolved
src/components/portal/portal.tsx Show resolved Hide resolved
@SergeyMyssak SergeyMyssak force-pushed the 707-Portale-element-to-any-referenced-DOM-element branch from d93e30b to 661f108 Compare May 16, 2023 04:04
@SergeyMyssak SergeyMyssak force-pushed the 707-Portale-element-to-any-referenced-DOM-element branch 3 times, most recently from 3b497a0 to f371264 Compare June 20, 2023 03:40
@wbeckler
Copy link

@BSFishy Are you happy with where this landed?

@BSFishy
Copy link
Contributor

BSFishy commented Jun 29, 2023

@BSFishy Are you happy with where this landed?

My concern with this PR is that it was meant to go into Dashboards but I believe it's breaking. @SergeyMyssak can confirm if that's the case. I spun up a Dashboards instance with this change and went to Stack Management and changed some settings to see the bottom bar, and nothing seemed to have broken, but if there's a chance that someone will break because of this, I don't want to put it into the 1.x line.

@SergeyMyssak SergeyMyssak force-pushed the 707-Portale-element-to-any-referenced-DOM-element branch from 05b6ac0 to ac5568d Compare July 1, 2023 04:19
@SergeyMyssak
Copy link
Collaborator Author

@BSFishy Are you happy with where this landed?

My concern with this PR is that it was meant to go into Dashboards but I believe it's breaking. @SergeyMyssak can confirm if that's the case. I spun up a Dashboards instance with this change and went to Stack Management and changed some settings to see the bottom bar, and nothing seemed to have broken, but if there's a chance that someone will break because of this, I don't want to put it into the 1.x line.

In the Dashboards, we have BottomBar in 3 places:

  1. advanced_settings
  2. data_source_management (create_data_source_wizard)
  3. data_source_management (edit_data_source_wizard)

The changes only concern the default values usePortal and affordForDisplacement. If we merge this PR, we will need to open a PR in Dashboards in which we will need to explicitly specify the usePortal prop in items 2 and 3, because it will have no default value. No change is required for item 1. If this is a blocker for merge in 1.x, better leave it for 2.0.

@SergeyMyssak SergeyMyssak force-pushed the 707-Portale-element-to-any-referenced-DOM-element branch from ac5568d to cf5351b Compare July 1, 2023 06:02
@SergeyMyssak @andreymyssak
Update CHANGELOG (opensearch-project#707)
30054ad 
Co-authored-by: Andrey Myssak <[email protected]>
Signed-off-by: Sergey Myssak <[email protected]>
@SergeyMyssak SergeyMyssak force-pushed the 707-Portale-element-to-any-referenced-DOM-element branch from cf5351b to 30054ad Compare July 1, 2023 06:03
BSFishy
BSFishy approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@BSFishy BSFishy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test suite passes. I think we can consider this something like "clarifying previously ambiguous functionality/requirements" based on #758 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BSFishy BSFishy BSFishy approved these changes

@seanneumann seanneumann Awaiting requested review from seanneumann

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@rednaksi91 rednaksi91 Awaiting requested review from rednaksi91

@KrooshalUX KrooshalUX Awaiting requested review from KrooshalUX

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@joshuarrrr joshuarrrr Awaiting requested review from joshuarrrr joshuarrrr is a code owner

@andreymyssak andreymyssak Awaiting requested review from andreymyssak

At least 1 approving review is required to merge this pull request.

Assignees

@joshuarrrr joshuarrrr

@BSFishy BSFishy

Labels
CCI College Contributor Initiative valued-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SergeyMyssak @wbeckler @BSFishy @joshuarrrr