Skip to content

portals page #73

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
Merged
merged 7 commits into from
Apr 20, 2020
Merged

portals page #73

merged 7 commits into from
Apr 20, 2020

Conversation

@nooblyf
Copy link
Contributor

@nooblyf nooblyf commented Mar 20, 2020

No description provided.

@netlify
Copy link

netlify bot commented Mar 20, 2020 •
edited
Loading

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
Copy link
Member

@arshadkazmi42 arshadkazmi42 left a comment

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.

@nooblyf
Copy link
Contributor Author

nooblyf commented Apr 4, 2020

@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
Copy link
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
Copy link
Member

@arshadkazmi42 arshadkazmi42 left a comment

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 requested changes Apr 19, 2020
View reviewed changes
Copy link
Member

@saranshkataria saranshkataria left a comment

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.

@saranshkataria
Copy link
Member

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

@saranshkataria saranshkataria saranshkataria approved these changes

@arshadkazmi42 arshadkazmi42 arshadkazmi42 approved these changes

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