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

feat(PPDSC-2005) Layer manager #146

Merged
merged 78 commits into from
Jul 25, 2022
Merged

feat(PPDSC-2005) Layer manager #146

merged 78 commits into from
Jul 25, 2022

Conversation

pdimova
Copy link

@pdimova pdimova commented Apr 4, 2022

PPDSC-2005

What

  1. Background - we need a better way to organize layers ( stacking context ) without using a z-index.
  2. What did you do: two new components Layer and Layer organizer
    2.1. Layer takes all children and renders them into a portal, attached to the document body or LayerOrganizer
    2.2. LayerOrganizer is a placeholder in which Layer can be rendered, without a LayerORganizer the Layers will be attached to the body
    2.3. Others:
    • Added NewskitProvided which includes: ThemeProvider, InstrumentationProvider, MediaQueryProvider & LayerOrganizer
    • withMediaQueryProvider removed from Select, since NewskitProvider contains MediaQueryProvider
    • Updated themeprovider/newskitprovider docs
    • Updated storybook config to use NewskitProvider
    • Updated test-utils to use NewskitProvider
  • SSR Consideration: React Portals don't render on the server ( they are client only feature ) which means during SSR the content of the Layers will be rendered in "normal" flow, and when client code kicks in, the layer content will be transferred via the portal at the end of the page ( Layer Organizer / document.body ). This behaviour can be seen in our home page using the Drawer ( pages/index.tsx )

  • breaking change: This is a breaking change since we remove Z-Index from components, and render them outside app "root". This means they can collision with other components with z-index ( non newskit ones )
    For example Virgin Radio:
    This is how broken is Drawer in Virgin Radio since there are components that have zIndex
    Screenshot 2022-04-21 at 13 00 13

In order to fix that we need to add <LayerOrganizer zIndex={99999} > so that create a new stacking context for our Layers.
Screenshot 2022-04-21 at 12 54 47

BREAKING CHANGE: Layers render outside "root" app

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@github-actions github-actions bot added the feature This change contains a new feature label Apr 4, 2022
Copy link
Contributor

@mutebg mutebg left a comment

Choose a reason for hiding this comment

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

wow, that is super simple and clever at the same time, great job @pdimova

@Vanals Vanals changed the base branch from develop to main April 11, 2022 16:33
@mutebg mutebg requested review from mutebg and Xin00163 July 21, 2022 07:25
@mutebg mutebg added ready for review Please assist in getting this reviewed and removed DO NOT MERGE Do not merge this PR labels Jul 21, 2022
Copy link
Contributor

@Xin00163 Xin00163 left a comment

Choose a reason for hiding this comment

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

We have now added z-index to new components like tooltip & popover. Do they need to be removed in this PR? @mutebg

@mutebg
Copy link
Contributor

mutebg commented Jul 22, 2022

We have now added z-index to new components like tooltip & popover. Do they need to be removed in this PR? @mutebg

We might do that in another ticket, don't want to spend another 1 month doing so, we also need to check how these components render in the portal, I assume it won't be a problem but

Xin00163
Xin00163 previously approved these changes Jul 22, 2022
@mutebg mutebg requested review from Xin00163 and mstuartf July 25, 2022 09:55
mstuartf
mstuartf previously approved these changes Jul 25, 2022
@mutebg mutebg merged commit 63c3556 into main Jul 25, 2022
@mutebg mutebg deleted the feat/PPDSC-2005-layer-manager branch July 25, 2022 13:27
Xin00163 pushed a commit that referenced this pull request Oct 17, 2022
* fix(PPDSC-1972): select panel - add zindex

* fix(PPDSC-1972): merge - update snapshots

* fix(PPDSC-1972): failing tests - mock datetime

* fix(PPDSC-1972): add comment

* feat(PPDSC-0000): wip

* feat(PPDSC-2005): add layer manager

* feat(PPDSC-2005): delete portal component

* feat(PPDSC-2005): add layers

* feat(PPDSC-2005): update layer organizer

* feat(PPDSC-2005): cleanup

* feat(PPDSC-2005): clean up & new story

* feat(PPDSC-2005): fixx select and inline prop on modal/drawer

* feat(PPDSC-2005): fix unit tests

* feat(PPDSC-2005): add comments

* feat(PPDSC-2005): test LM on docs site

* feat(PPDSC-2005): add tests

* feat(PPDSC-2005): add unit tests to layer

* feat(PPDSC-2005): fix unit tests

* feat(PPDSC-2005): fix e2e tests

* feat(PPDSC-2005): add srr

* feat(PPDSC-2005): custom portal

* feat(PPDSC-2005): add portal

* feat(PPDSC-2005): improvements

* feat(PPDSC-2005): update layer manager

* feat(PPDSC-2005): fix ts

* feat(PPDSC-2005): fix e2e tests

* feat(PPDSC-2005): fix coverage

* feat(PPDSC-2005): fix snapshots

* feat(PPDSC-2005): fix LO

* feat(PPDSC-2005): fix e2e tests

* feat(PPDSC-2005): clean up

* feat(PPDSC-2005): clean up

* feat(PPDSC-2005): clean up

* feat(PPDSC-2005): fix snapshots

* feat(PPDSC-2005): add NewskitProvider

* feat(PPDSC-2005): add select story

* feat(PPDSC-2005): add warnings

* feat(PPDSC-2005): remove mediaQueryProvider from select

* feat(PPDSC-2005): update test-utils with newskitprovider

* feat(PPDSC-2005): update newskit/theme provider docs

* feat(PPDSC-2005): update snapshots

* feat(PPDSC-2005): fix coverage

* feat(PPDSC-2005): fix types and tests cleanup

* feat(PPDSC-2005): update e2e tests

* feat(PPDSC-2005): fix snapshots

* feat(PPDSC-2005): fix coverage

* feat(PPDSC-2005): fix unit tests

* feat(PPDSC-2005): ignore test-utils from coverage

* feat(PPDSC-2005): address comments

* feat(PPDSC-2005): add docs

* feat(PPDSC-2005): update snapshots

* feat(PPDSC-2005): remove comment

* feat(PPDSC-2005): update select snapshots

* feat(PPDSC-2005): update docs snapshots

* feat(PPDSC-2005): update mediaQueruContext

* feat(PPDSC-2005): update docs

* feat(PPDSC-2005): rename file

* feat(PPDSC-2005): rename NewskitProvider to NewsKitProvider

* feat(PPDSC-2005): fix typo

* feat(PPDSC-2005): add waitBeforeCapture

Co-authored-by: Stoyan Delev <[email protected]>
Co-authored-by: Stoyan Delev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature This change contains a new feature ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants