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

[core] feat: Overlay2 component #6656

Merged
merged 24 commits into from
Jan 24, 2024
Merged

[core] feat: Overlay2 component #6656

merged 24 commits into from
Jan 24, 2024

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Jan 16, 2024 •
edited

Fixes #3979

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Create new Overlay2 component which has the following improvements compared to Overlay:
  • Deprecate Overlay
  • Add Overlay2 example with <StrictMode> enabled
  • Add unit test suite for Overlay2 (mostly ported from overlayTests.tsx, with some modifications to adjust for the new constraints on children - see migration guide)
  • Migrate straightforward overlay-based components to use Overlay2
    • Dialog
    • Drawer
    • Omnibar
  • Migrate complex overlay-based components to use Overlay2
    • OverlayToaster
    • Popover
  • Create Toast2 component (forwards DOM ref to rendered DOM node), deprecate Toast
  • Add documentation for Overlay2, update OverlayToaster documentation

TODO (likely in a future PR):

Reviewers should focus on:

No regressions in overlay-based components

Screenshot

Overlay2 in docs-app:

Screen Recording 2024-01-23 at 10 36 48 AM

OverlayToaster using Overlay2 and Toast2 in docs-app:

Screen Recording 2024-01-23 at 10 38 05 AM

Toast2 in demo-app:

image

Child ref docs:

image
image

@adidahiya
Copy link
Contributor Author

fix compilation, add docs, strict mode compat

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

fix Overlay2 iso test

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

migrate overlay components to use Overlay2

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

fix (almost) all tests

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

Merge remote-tracking branch 'origin/develop' into ad/overlay2

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

update Overlay2 docs

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

adidahiya
adidahiya commented Jan 22, 2024
View reviewed changes
Copy link
Contributor Author

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

need to deprecate <Toast> and update OverlayToaster docs to mention <Toast2>

packages/core/src/common/utils/jsUtils.ts Show resolved Hide resolved
packages/core/src/components/dialog/dialog.tsx Show resolved Hide resolved
packages/core/src/components/overlay/overlay.md Outdated Show resolved Hide resolved
packages/core/src/components/overlay2/overlay2.md Outdated Show resolved Hide resolved
@adidahiya adidahiya marked this pull request as ready for review January 22, 2024 22:29
@adidahiya
Copy link
Contributor Author

fix docs TODO, improve errors

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya mentioned this pull request Jan 22, 2024
Closed
@adidahiya
Copy link
Contributor Author

fix lint

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

adidahiya
adidahiya commented Jan 23, 2024
View reviewed changes
packages/core/src/components/overlay2/overlay2.tsx
const getLastOpened = () => openStack[openStack.length - 1];

/**
* HACKHACK: ugly global state manipulation should move to a proper React context
Copy link
Contributor Author

@adidahiya adidahiya Jan 23, 2024 •
edited

Choose a reason for hiding this comment

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

consider fixing this in this PR and biting the bullet with the necessary <OverlayProvider> break

edit: this PR is already quite large, I could add the context/provider in a follow-up before cutting the next release

packages/core/src/components/overlay2/overlay2.tsx
);

React.useEffect(() => {
setRef(ref, instance);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consider useImperativeHandle to do this instead

@adidahiya adidahiya merged commit 9dfba77 into develop Jan 24, 2024
11 of 12 checks passed
@adidahiya adidahiya deleted the ad/overlay2 branch January 24, 2024 13:27
This was referenced Jan 24, 2024
Merged
Merged
Open
Closed
Closed
Closed
@gluxon gluxon mentioned this pull request May 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Removing findDOMNode
1 participant
@adidahiya