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.

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

[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

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.

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

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

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

);

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing findDOMNode
1 participant