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

fix(popup): Remove containerElement prop from Popper #1524

Merged
merged 6 commits into from
Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions modules/codemod/lib/v7/removePropFromPopper.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {API, FileInfo, Options, JSXElement} from 'jscodeshift';
import {getImportRenameMap} from './utils/getImportRenameMap';
import {hasImportSpecifiers} from '../v6/utils';

const popupBarPackage = '@workday/canvas-kit-react/popup';

export default function transformer(file: FileInfo, api: API, options: Options) {
const j = api.jscodeshift;

const root = j(file.source);

if (!hasImportSpecifiers(api, root, popupBarPackage, ['Popper'])) {
return file.source;
}

const {importMap, styledMap} = getImportRenameMap(j, root, popupBarPackage);

root
.find(
j.JSXElement,
(value: JSXElement) =>
value.openingElement.name.type === 'JSXIdentifier' &&
(value.openingElement.name.name === importMap.Popper ||
value.openingElement.name.name === styledMap.Popper)
)
.forEach(nodePath => {
const attributes = nodePath.value.openingElement.attributes;
// Remove containerElement prop from Popper component
if (attributes) {
nodePath.value.openingElement.attributes = attributes.filter(item =>
item.type === 'JSXAttribute' &&
item.name.type === 'JSXIdentifier' &&
['containerElement'].includes(item.name.name)
? false
: true
);
}
});

return root.toSource();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding this!

39 changes: 39 additions & 0 deletions modules/codemod/lib/v7/spec/removePropFromPopper.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import {expectTransformFactory} from './expectTransformFactory';
import transform from '../removePropFromPopper';
import {stripIndent} from 'common-tags';

const expectTransform = expectTransformFactory(transform);

describe('removePropFromPopper', () => {
it('should remove containerElement prop from Popper component', () => {
const input = stripIndent`
import {Popper} from '@workday/canvas-kit-react/popup';

<Popper containerElement="any element here" />
`;

const expected = stripIndent`
import {Popper} from '@workday/canvas-kit-react/popup';

<Popper />
`;

expectTransform(input, expected);
});

it('should remove containerElement prop from Popper component imported from main package', () => {
const input = stripIndent`
import {Popper} from '@workday/canvas-kit-react';

<Popper containerElement="any element here" />
`;

const expected = stripIndent`
import {Popper} from '@workday/canvas-kit-react';

<Popper />
`;

expectTransform(input, expected);
});
});
9 changes: 8 additions & 1 deletion modules/docs/mdx/7.0-MIGRATION-GUIDE.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ any questions about the update.
- [Icons](#icons)
- [ActionBar Component Updates](#actionbar-component-updates)
- [Status Indicator Width](#status-indicator-width)
- [Popup Cards](popup-cards)
- [Popup Cards](#popup-cards)
- [Popper Props Update](#popper-props-update)
- [Component Promotions](#component-promotion)

## Codemod

Expand Down Expand Up @@ -463,6 +465,11 @@ If your code contains any hacks to make a `Modal` overflow, these hacks should n
`max-height` of the `Modal.Body` element using calculations. These should be removed. The
`Popup.Card` now has a max height and the `Popup.Body` height is automatically calculated.

## Popper Props Update

We removed the `containerElement` prop from Popper component because it's no longer needed with
Fullscreen API.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a codemod for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked usage of Popper, so it will be good to add codemod for making migration easier. Thanks for catching it, I'll add it


## Component Promotions

After some [assessment](https://github.com/Workday/canvas-kit/issues/1395) we've decided to promote
Expand Down
13 changes: 2 additions & 11 deletions modules/react/popup/lib/Popper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,6 @@ export interface PopperProps {
* `anchorElement`.
*/
children: ((props: {placement: Placement}) => React.ReactNode) | React.ReactNode;
/**
* The element that contains the portal children when `portal` is true. It is best to not define
* this unless you know what you're doing. Popper works with a PopupStack and in order for
* z-indexes to work correctly, all Popups on your page should live on the same root element
* otherwise you risk running into rendering issues:
* https://philipwalton.com/articles/what-no-one-told-you-about-z-index/
*/
containerElement?: Element | null;
/**
* When provided, this optional callback will be used to determine positioning for the Popper element
* instead of calling `getBoundingClientRect` on the `anchorElement` prop. Use this when you need
Expand Down Expand Up @@ -62,7 +54,7 @@ export interface PopperProps {
*/
popperOptions?: Partial<PopperOptions>;
/**
* If true, attach the Popper to the `containerElement`. If false, render the Popper within the
* If false, render the Popper within the
* DOM hierarchy of its parent. A non-portal Popper will constrained by the parent container
* overflows. If you set this to `false`, you may experience issues where you content gets cut off
* by scrollbars or `overflow: hidden`
Expand Down Expand Up @@ -113,7 +105,6 @@ const OpenPopper = React.forwardRef<HTMLDivElement, PopperProps>(
onPlacementChange,
children,
portal,
containerElement,
popperInstanceRef,
}: PopperProps,
ref
Expand Down Expand Up @@ -185,7 +176,7 @@ const OpenPopper = React.forwardRef<HTMLDivElement, PopperProps>(
return contents;
}

return ReactDOM.createPortal(contents, containerElement || stackRef.current!);
return ReactDOM.createPortal(contents, stackRef.current!);
}
);

Expand Down