Skip to content

portals page#73

Merged
saranshkataria merged 7 commits into
reactjs:masterfrom
nooblyf:portals-page
Apr 20, 2020
Merged

portals page#73
saranshkataria merged 7 commits into
reactjs:masterfrom
nooblyf:portals-page

Conversation

@nooblyf

@nooblyf nooblyf commented Mar 20, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@netlify

netlify Bot commented Mar 20, 2020 •
edited
Loading

Copy link
Copy Markdown

Deploy preview for hi-reactjs ready!

Built with commit b5d5262

https://deploy-preview-73--hi-reactjs.netlify.app

@nooblyf nooblyf changed the title complete portals page portals page Mar 20, 2020
@arshadkazmi42 arshadkazmi42 added the 1st Review First phase of review label Apr 4, 2020
arshadkazmi42
arshadkazmi42 requested changes Apr 4, 2020
View reviewed changes

@arshadkazmi42 arshadkazmi42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for taking this up and great start.
Added some reviews, once these are fixed, it will be good to go from my end.
Also, do read #23 to know about our review process.

Comment thread content/docs/portals.md
Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md
Comment thread content/docs/portals.md
Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md
Comment thread content/docs/portals.md Outdated
@nooblyf

nooblyf commented Apr 4, 2020

Copy link
Copy Markdown
Contributor Author

@arshadkazmi42 I wrote "portal(s)" in english because it is listed in the glossary under No translation required and also method name is createPortal() so I think it will make more sense, no?

@arshadkazmi42

arshadkazmi42 commented Apr 5, 2020

Copy link
Copy Markdown
Member

@arshadkazmi42 I wrote "portal(s)" in english because it is listed in the glossary under No translation required and also method name is createPortal() so I think it will make more sense, no?

@Mayankverr Actually you are right. I completely forgot about it. Please ignore all my comments related to portals

arshadkazmi42
arshadkazmi42 approved these changes Apr 6, 2020
View reviewed changes

@arshadkazmi42 arshadkazmi42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work. 🎉

Thank you for working on the fixes. For next steps, there will be another phase of review done by @saranshkataria , post that review we will get this merged

@arshadkazmi42 arshadkazmi42 added 2nd Review Second phrase of review and removed 1st Review First phase of review labels Apr 6, 2020
saranshkataria
saranshkataria suggested changes Apr 19, 2020
View reviewed changes

@saranshkataria saranshkataria left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

added comments, once these are resolved, this is good to merge.

Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md
Comment thread content/docs/portals.md Outdated
Comment thread content/docs/portals.md Outdated
@saranshkataria

saranshkataria commented Apr 20, 2020

Copy link
Copy Markdown
Contributor

great work @Mayankverr. Thanks! Merging this in 🥳

@saranshkataria
saranshkataria merged commit 71b9aa7 into reactjs:master Apr 20, 2020
@tesseralis tesseralis mentioned this pull request Apr 20, 2020
Closed
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arshadkazmi42 arshadkazmi42 arshadkazmi42 approved these changes
+1 more reviewer
@saranshkataria saranshkataria saranshkataria approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2nd Review Second phrase of review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nooblyf @arshadkazmi42 @saranshkataria