docs(header): replace logo.png with logo.svg #2874
Conversation
|
@petebacondarwin I added an img.onerror attribute to handle IE 8 fallback. Everything should be good to go. I don't know why the jquery file permissions were changed. I didn't touch those files. If there's anything I can do to remove those from the pull request, please let me know. Thanks! |
| @@ -118,7 +118,11 @@ | |||
| <div class="navbar-inner"> | |||
| <div class="container"> | |||
| <a class="brand" href="http://angularjs.org" style="padding-top: 6px; padding-bottom: 0px;"> | |||
| <img class="AngularJS-small" src="http://angularjs.org/img/AngularJS-small.png"> | |||
There was a problem hiding this comment.
Can we make these src URLs relative so that they work when used locally or on CI server. See http://ci.angularjs.org/job/angular.js-pete/181/artifact/build/docs/api
There was a problem hiding this comment.
I'd be happy to. I'm only using absolute paths because they were like that when I got there. =) Relative is much nicer.
Your link is 404ing, but this one is still using the PNG. Is that link being written by a build script?
Also, how would you like that path to be written? I imagine it should be something like "/img/path.svg", but then you'd have a hole in your Jenkins build, since docs isn't mounted on /. Is there a variable I could use, like {{ IMG_PATH }}?
There was a problem hiding this comment.
Yours 404s as well :-) This is because Angular is rewriting the URL and the CI server is not setup to redirect deep URLs!
Try this, which is probably the same at the moment as yours anyway: http://ci.angularjs.org/job/angular.js-pete/181/artifact/build/docs/index.html
These pages are generated on every build. The CI server watches my github repos and builds and tests on changes.
You can do this locally with grunt docs or something.
The relative path can actually just be img/path.svg, without the / at the start. This works for the links in the navigation menu.
There was a problem hiding this comment.
I looked through the source in /docs and didn't see any part that would dynamically rewrite the relative URL. I also tried grunt docs, and it was still relative after the build.
I cringed when I first saw the hardcoded link too, but I think I understand now why it's done. The images on angularjs.org probably don't change much, so even if you are technically pulling in a production image, everything still looks OK on staging. On the other hand, since the header is in index.html (and not one of the ngdoc pages), it doesn't get built, so there's no opportunity in the current setup to rewrite that link.
Moreover, since the fallback is done in JS, we'd need a token (like {{ IMAGES_PATH }}) to be able to make sure the base URL was put in the correct place in both the src and onerror attributes. Another option would be to use a <base/> tag in index.html.
Please let me know if I am mistaken, but I think what's best for now is to follow the existing convention and use the hard-coded link, unless there's an opportunity to let the build script handle it that I don't know about.
|
@appsforartists - |
|
I didn't see the base tag - where is it defined? It's an SVG version of the EPSes in the GitHub repo, and its dimensions are the same. The width and height in the browser are set in docs.css: Is docs.css loading for you? |
|
@appsforartists can you please rebase and make the suggested changes? |
|
@btford I'm not sure what changes you're referring to. |
|
* Improved developer guide, directive unit testing documentation code with scope expression * Removed documentation block with nothing on it
- add toThrowNg matcher
By appending directive-start and directive-end to a directive it is now possible to have the directive act on a group of elements. It is now possible to iterate over multiple elements like so: <table> <tr ng-repeat-start="item in list">I get repeated</tr> <tr ng-repeat-end>I also get repeated</tr> </table>
…lay for and transitions and animations together
jQuery's API for removeData allows a second 'name' argument to just remove the property by that name from an element's data. The absence of this argument was causing some features not to work correctly when combining multiple directives, such as ng-click, ng-show, and ng-animate.
Add a reference to the blog at the documentation.
If you `cd` into the repo, `validate-commit-msg.js` will be in the root of it.
Before the Develop drop down menu items were hard coded with an absolute url, which meant that they did not work correctly on local or ci server builds.
fromJSON() should be fromJson()
The current logo looks awful on high-density displays. SVG is a better choice because it can scale to any resolution without increasing file size. Amending angular#2775 to add support for IE 8 by falling back to existing PNG with img.onerror Using relative URLs as directed by @btford and @petebacondarwin.
|
Alright, @btford, I've made the paths relative and squashed the commits. Can we please merge this now? |
|
I took the last commit and tested it locally. It doesn't look right in IE, (either IE10 or in IE8 mode), see these screenshots:
|
|
@appsforartists - have you been able to look at these IE problems? |
|
@petebacondarwin I haven't, and I honestly don't know that I'm going to any time soon. This was supposed to be a quick patch, and it's been thrown back at me every two weeks for two months. I don't have an IE machine handy to do the testing, but it's a simple Before I touched it, that img used an absolute path (I changed to a relative path at your request). Is the relative path causing the IE8 broken img? If you'd like the homepage to look nice on high-resolution displays, feel free to take this PR and make whatever changes you like to it. Otherwise, you can close it. This ping-pong of trivial changes is wasting both of our time on what should have been a 10 minute job. I mean no disrespect, but I don't have time to babysit this patch right now. |
|
@appsforartists I understand what you are saying, but please also understand that your simple PR is making things worse not better, and pretty much every PR in our long PR queue has similar issues - the goal of the PR is great, but it's just not good enough in its current state. We try to fix up PRs when we understand the issues, but sometimes the issues require too much of our time and that's when we ask contributors to fix the issues we identified. I'm going to spend 10minutes trying to figure out what's going on. If I can't fix it, I'll close this PR. Again, please understand that we appreciate your help and see that you are trying to make things better, but there are issues with your change and we have way too many PRs with similar issues as ours that are occupying too much of our time. |
|
it has been way more than 10 minutes. I made quite a few changes and pretty much the only thing that was left from your original commit was on svg image. I'm not accepting the svg images for the images/ directory because the were not created from our master angular logo. the docs logo is a one off, so we are not going to make anything worse by switching it over to svg. I removed the png fallback as the png image has been removed as well. thanks for the contribution. out of curiosity: how did you create the svg images? |
The current logo looks awful on high-density displays. SVG is a better choice because it can scale to any resolution without increasing file size. Amending #2775 to add support for IE 8 by falling back to existing PNG with img.onerror Using relative URLs as directed by @btford and @petebacondarwin. (commit by Brenton Simpson - @appsforartists) Closes #2874 Conflicts: docs/src/templates/css/docs.css docs/src/templates/index.html
|
Thanks Igor. I didn't mean to come across as hostile. I certainly understand you guys are inundated with PRs, I just didn't have the cycles to play PR pong on this one. (Perhaps it would be better next time to schedule some mutual availability on IRC or IM to discuss changes on these sorts of issues. If I had a clearer idea of what changes were wanted from the offset, and a more immediate feedback loop, this would have been a lot easier.) I don't know why this caused such a headache. All I did was swap the img.@src with an SVG. When he sent the screenshot of the image being too large in IE, I moved the dimensions into the markup to avoid any FOUCs. (I see you've put them back in the CSS.) The PNG fallback was on his request. The padding et. al. were like that when I got there, but have now moved into CSS. I don't mean to be deflecting blame onto anybody; I just don't understand how this snowballed. My original pull request was very small and very similar to yours (an SVG in the src and the dimensions in CSS). The relative paths and moving of padding into a CSS class are both orthagonal to the image swap, but got bundled into this anyway. You and @petebacondarwin seem to have differing opinions on whether a PNG fallback is needed. In any case, I'm happy to have helped. I'm just sad for everybody's sake that what should have been a small easy change caused so much churn, and hope I can offer feedback to help avoid this situation next time. As for the SVGs, I just took the EPSes and converted them. I exported versions with the Google tagline to match the EPSes exactly, and ones without for situations like the header where it doesn't fit. |
|
@appsforartists - Hi Brenton, I hope you are enjoying a hi-fidelity experience on your retina screen now! I am also sorry that this PR seems to have become somewhat drawn out. In my defence, all I ever did was point out that SVG is not supported on IE8, which we are expected to support; and then to ask that you make the URLs relative so they didn't break on the CI server. Not exactly a lot to ask! The fact that this then turned out to be harder to fix than expected was unfortunate. It seems that @IgorMinar has taken the view that a broken logo on IE8 is not an issue. A call that I was not in a position to make. Here is a screenshot of the current from IE8. It seems like there is a whole load of broken stuff there in any case: |
|
@petebacondarwin no need for defense - I don't think anyone did anything wrong. =) The problem here is mostly process. I appreciate that you review PRs and notify the author when something goes wrong. In retrospect, it may go smoother to say "I'm going to be on IRC tomorrow from X-Y. Feel free to PM me when you've fixed this and we'll look at it together." That's especially effective in cases like this one, where you're testing in an environment the author might not have access to. It's totally fair to say "this PR doesn't look right on an environment we support" or "while you're in there, can you please make this small tweak as well." The problem here was all timing. If the author is still in the right context (both mentally and environmentally) to work on a an Angular PR, these are all easy changes. However, when weeks go by in between, then it's "remember what you were doing, pull up the right files, spin up the right devices, make the requested changes, test, resubmit, and hope it passes." When the changes are relatively small, you're asking the author to go through a lot of overhead for something that you can easily try as the reviewer when you've already got your environment setup correctly. If you change works, you can amend the patch and close the issue. If it doesn't, you're in a better position to raise the bug with the original author. I hope that's helpful feedback. I don't mean to be out-of-line or accusing anybody of anything. I just want to help make the Angular enhancement experience great for both you guys and for the dev community at large so it can keep growing to be an awesome framework. =) |
|
I don't think there are many people with an XP box running IE8 kicking On 1 August 2013 18:12, Brenton Simpson notifications@github.com wrote:
|
The current logo looks awful on high-density displays. SVG is a
better choice because it can scale to any resolution without
increasing file size.
Amending #2775 to add support for IE 8 by falling back to existing PNG
with img.onerror