-
Notifications
You must be signed in to change notification settings - Fork 12k
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
Feat new landing #6144
Conversation
There was a problem hiding this 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!!!'); |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damnn forgot those 😭
6bd0b7e
to
97be4f5
Compare
<h1> | ||
Welcome to {{title}}!! | ||
</h1> | ||
<img src="https://angular.io/resources/images/logos/angular/angular.png" /> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
858c776
to
1c86f97
Compare
@@ -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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
836e95b
to
6246a99
Compare
<h1> | ||
Welcome to {{title}}!! | ||
</h1> | ||
<img width="300" src="https://angular.io/resources/images/logos/angular/angular.svg" /> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Please rebase on top of master. |
6246a99
to
9e4365b
Compare
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 |
9e4365b
to
16dad7c
Compare
CLA should be ignored. I verified it's good. |
(the CLA status will never resolve, I'm going to merge this once all reviewers are okay) |
16dad7c
to
5d18b21
Compare
CLAs look good, thanks! |
5d18b21
to
0b66e4b
Compare
173d84b
to
d662833
Compare
There was a problem hiding this 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.
d662833
to
4d4a4d7
Compare
@@ -12,11 +12,13 @@ | |||
}, | |||
"private": true, | |||
"dependencies": { | |||
"@angular/animations": "^4.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fix #5928
Fix #6125