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

Feat new landing #6144

Merged
merged 5 commits into from
May 9, 2017
Merged

Feat new landing #6144

merged 5 commits into from
May 9, 2017

Conversation

sumitarora
Copy link
Contributor

@sumitarora sumitarora commented May 1, 2017

Fix #5928
Fix #6125

@sumitarora sumitarora requested review from hansl and Brocco May 1, 2017 19:43
Copy link
Contributor

@Brocco Brocco left a comment

Choose a reason for hiding this comment

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

A few coments

@@ -9,6 +9,6 @@ describe('<%= htmlComponentName %> App', () => {

it('should display message saying app works', () => {
page.navigateTo();
expect(page.getParagraphText()).toEqual('<%= prefix %> works!');
expect(page.getParagraphText()).toEqual('Welcome to <%= prefix %> works!!!');
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to remove works

</h1>
<img src="https://angular.io/resources/images/logos/angular/angular.png" />
</div>
<h2>Here are some links to help you start: </h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're calling them links in text, please make them links with URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damnn forgot those 😭

<h1>
Welcome to {{title}}!!
</h1>
<img src="https://angular.io/resources/images/logos/angular/angular.png" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if this could/should be an SVG inlined? So it doesn't depend on an internet connection (or CORS) :)

https://angular.io/resources/images/logos/angular/angular.svg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure or I can add it to assets

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @hansl please just add it in-line, this way when it is removed, it's completely gone, rather than having to change it here in markup as well as removing the file from the assets dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sumitarora sumitarora force-pushed the feat-new-landing branch 2 times, most recently from 858c776 to 1c86f97 Compare May 2, 2017 00:28
@@ -9,6 +9,6 @@
<link rel="icon" type="image/x-icon" href="favicon.ico">
</head>
<body>
<<%= prefix %>-root>Loading...</<%= prefix %>-root>
<<%= prefix %>-root></<%= prefix %>-root>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of removing the helpful message?

Copy link
Contributor

Choose a reason for hiding this comment

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

It conveys a message that it will take a while for Angular to load which is just not true so the decision was made to remove it by default... if you'd like to add it in you certainly can.

@sumitarora sumitarora force-pushed the feat-new-landing branch 3 times, most recently from 836e95b to 6246a99 Compare May 2, 2017 18:24
<h1>
Welcome to {{title}}!!
</h1>
<img width="300" src="https://angular.io/resources/images/logos/angular/angular.svg" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you inline the image? It's SVG so you can just copy its content in the HTML, and that way even without a network connection you get it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, please embed the SVG or inline a base64 encoded image for easier removal.

<h1>
{{title}}
</h1><% if (routing) { %>
<div style="text-align:center">
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment at the top that this whole content can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@hansl hansl changed the base branch from 1.1.x to master May 4, 2017 00:32
@hansl
Copy link
Contributor

hansl commented May 4, 2017

Please rebase on top of master.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels May 4, 2017
@sumitarora sumitarora closed this May 4, 2017
@sumitarora sumitarora reopened this May 4, 2017
@sumitarora sumitarora closed this May 4, 2017
@sumitarora sumitarora reopened this May 4, 2017
@hansl
Copy link
Contributor

hansl commented May 4, 2017

CLA should be ignored. I verified it's good.

@hansl hansl added cla: yes and removed cla: no labels May 4, 2017
@hansl
Copy link
Contributor

hansl commented May 4, 2017

(the CLA status will never resolve, I'm going to merge this once all reviewers are okay)

@googlebot
Copy link

CLAs look good, thanks!

@sumitarora sumitarora force-pushed the feat-new-landing branch 2 times, most recently from 173d84b to d662833 Compare May 5, 2017 16:09
Brocco
Brocco previously approved these changes May 8, 2017
Copy link
Contributor

@Brocco Brocco left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for making the changes... please resolve the external Angular logo image issue that @hansl commented about.

@@ -12,11 +12,13 @@
},
"private": true,
"dependencies": {
"@angular/animations": "^4.0.0",
Copy link
Contributor

@dave11mj dave11mj May 9, 2017

Choose a reason for hiding this comment

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

Could we add this dependency on #5786 ? Or update this PR to integrate BrowserAnimationsModule? simply adding the dependency to the package wouldn't make it any easier to use animations or angular material as requested on #5928. It would also take up space from apps that do not need animations.

Also, does not fix #5785 which was closed in favor of #5928

Copy link
Contributor

Choose a reason for hiding this comment

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

@dave11mj About #5786; importing Animations into the NgModule has a lot of impact on the rendering and it can negatively affect performance if you're not using animations. As such we just add it to the package.json to facilitate you importing it, if you need to, but we don't force you to remove it if you don't. It will not be included in the build if you don't need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that, which is why a flag makes sense for it. Where I am confused is what benefit does it provide to include on the package.json which would download it to offline caches and such for projects that may not use animations at all.

Also if its preferred to always have @angular/animations whether is needed or not for #5928 then we could at least re-open #5785 to tackle optionally importing browser animations.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add tslint-language-service to dependencies Add dependency on @angular/animations for ng new projects
7 participants