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

Add page for Angular #76

Merged
merged 89 commits into from Apr 12, 2019
Merged

Add page for Angular #76

merged 89 commits into from Apr 12, 2019

Conversation

RonakLakhotia
Copy link
Contributor

@RonakLakhotia RonakLakhotia commented Feb 18, 2019

Fixes #70

@damithc
Copy link
Contributor

damithc commented Feb 20, 2019

@RonakLakhotia The structure looks fine. Can proceed to get peer reviews. Remember to credit original source when reusing assets from elsewhere.

@RonakLakhotia
Copy link
Contributor Author

@jacoblipech can review this PR?

Copy link
Contributor

@jacoblipech jacoblipech left a comment

Choose a reason for hiding this comment

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

LGTM! It is quite well-documented, just certain comments to consider. It will be good if you can read through one more time to check for line spacing, missing comma and minor grammar errors.

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jacoblipech jacoblipech left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! LGTM! minor spacing problem which you can settle or not.

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Added a few quick comments to the top portion. Take another critical look at the rest yourself and fix things that I'm likely to object to, based on my current comments. For example, don't make expansive claims as if you are selling Angular. Give an independent unbiased view.

_markbind/navigation/mainNav.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@damithc
Copy link
Contributor

damithc commented Mar 1, 2019

If you want to use marketing claims in the article e.g., effortless, trivial, etc., simply quote the claim from a credible source, even Angular website themselves, and cite the source. That way, you are not the one making the claim.

@damithc
Copy link
Contributor

damithc commented Mar 1, 2019

Also, wanted to clarify, if for instance I am using a term like MVC in my article. Is it fine to provide a link to another resource or should I explain MVC in the article?

Ideal, avoid using the term.
Next best, provide a simple explanation e.g., as a tooltip or a popover
Failing the above, provide a link to a good concise explanation elsewhere. It is not good if the reader has to read another big article before proceeding with yours though.

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

A preliminary review of the top portion. Revise the whole article to match the quality indicated by the review comments. Writing is hard.; buckle down for a long ride :-p

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@damithc
Copy link
Contributor

damithc commented Mar 8, 2019

@jacoblipech can do another review as the content has changed since your last review? Refer my review comments and calibrate your review to that depth. The deeper your review is, the better the final outcome will be.

@damithc
Copy link
Contributor

damithc commented Mar 10, 2019

Use Fixes #... in the PR description so that GitHub can auto-close the issue when the PR is merged.

Copy link
Member

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

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

Good effort in the writeup! Refer to the suggestions made 😄

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jacoblipech jacoblipech left a comment

Choose a reason for hiding this comment

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

LGTM! Despite using Angular in Teammates, I still learnt some new knowledge from reading it! Minor nits. Also, the "Given below..." phrasings feel quite weird to me but it may be just me, so up to you!

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@RonakLakhotia
Copy link
Contributor Author

@jacoblipech appreciate your review and inputs in this page!

@RonakLakhotia
Copy link
Contributor Author

@tanhengyeow sorry for the nagging, but still waiting on your final approval. :P

Copy link
Member

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

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

Looks much better now, great job! Just some nits.

Also, in your first section What is Angular?, you mentioned about Templates, Components, and Dependency Injection. I feel it might nice to put clear sub-headings for them so that it is easier to follow.

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@RonakLakhotia
Copy link
Contributor Author

@tanhengyeow thanks for the suggestions. Added clear headers for templates components and DI.
Can you take another look

@RonakLakhotia
Copy link
Contributor Author

@tanhengyeow could you look at it again, just a bit worried about the time constraints :P

Copy link
Member

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

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

LGTM! Just some final nits, thanks for the effort put into this chapter!

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@RonakLakhotia
Copy link
Contributor Author

@jacoblipech @tanhengyeow appreciate your reviews and thank you for the effort put into this!
@damithc your final call?

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Looking good now. As we spent so much time on this, why not make it near perfect so that it can serve a a model for future authors? here are few more things to consider.

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@RonakLakhotia
Copy link
Contributor Author

@damithc thanks for suggestions 👍 Updated the changes

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

More nits

contents/javascript/Javascript-framework-Angular.md Outdated Show resolved Hide resolved
@damithc damithc merged commit f699c91 into se-edu:master Apr 12, 2019
@damithc
Copy link
Contributor

damithc commented Apr 12, 2019

Merged and deployed. Good work @RonakLakhotia
Thanks to the reviewers too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduction to Angular
5 participants