Skip to content

[Portal] Render single div for container element #3465

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
Closed
wants to merge 6 commits into from
Closed

[Portal] Render single div for container element #3465

wants to merge 6 commits into from

Conversation

@dleroux
Copy link
Contributor

@dleroux dleroux commented Oct 13, 2020

WHY are these changes introduced?

Fixes #0000

WHAT is this pull request doing?

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2020 •
edited
Loading

🟡 This pull request modifies 5 files and might impact 60 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 60)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Pagination/tests/Pagination.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Portal/Portal.tsx (total: 60)

Files potentially affected (total: 60)

🧩 src/components/Portal/tests/Portal.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Tabs/tests/Tabs.test.tsx (total: 0)

Files potentially affected (total: 0)

qq99
qq99 reviewed Oct 20, 2020
View reviewed changes
src/components/Portal/Portal.tsx Outdated
: getUniqueID();

componentDidMount() {
const constainerNode = document.getElementById(UNIQUE_CONTAINER_ID);
Copy link
Contributor

@qq99 qq99 Oct 20, 2020

Choose a reason for hiding this comment

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

Suggested change
const constainerNode = document.getElementById(UNIQUE_CONTAINER_ID);
const containerNode = document.getElementById(UNIQUE_CONTAINER_ID);

Copy link
Contributor Author

@dleroux dleroux Oct 20, 2020

Choose a reason for hiding this comment

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

🙈

@alex-page alex-page changed the title [WIP] [Portal] Render single div for container element [Portal] Render single div for container element Oct 21, 2020
@alex-page alex-page marked this pull request as ready for review October 21, 2020 15:12
@alex-page alex-page self-requested a review October 21, 2020 15:22
alex-page
alex-page reviewed Oct 21, 2020
View reviewed changes
UNRELEASED.md
### Enhancements

Updated `Textfield` with a `type` of `number` to not render a spinner if step is set to `0` ([#3477](https://github.com/Shopify/polaris-react/pull/3477))
- Changed `Portal` to render all `Portals` in a single div ([#3465](https://github.com/Shopify/polaris-react/pull/3465))
Copy link
Member

@alex-page alex-page Oct 21, 2020

Choose a reason for hiding this comment

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

Do we want to add the conversion to functional component

Copy link
Contributor Author

@dleroux dleroux Oct 21, 2020

Choose a reason for hiding this comment

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

👍

kyledurand
kyledurand approved these changes Oct 21, 2020
View reviewed changes
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👏

dleroux
dleroux commented Oct 21, 2020
View reviewed changes
@dfmcphee dfmcphee mentioned this pull request Oct 21, 2020
Merged
6 tasks
@alex-page
Copy link
Member

alex-page commented Oct 22, 2020

Closing this for #3544

@alex-page alex-page closed this Oct 22, 2020
@alex-page alex-page deleted the add-container branch October 22, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kyledurand kyledurand kyledurand approved these changes

@alex-page alex-page alex-page approved these changes

@martenbjork martenbjork Awaiting requested review from martenbjork

+1 more reviewer

@qq99 qq99 qq99 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dfmcphee dfmcphee

@dleroux dleroux

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dleroux @alex-page @qq99 @kyledurand @dfmcphee