Add HNPWA entry using Polymer Starter Kit #55
Conversation
There was a problem hiding this comment.
All in all this looks like a solid app so thank you for submitting this 🎉. It's mobile responsive, seems quite fast and is an awesome build!
Just a few tiny things with the PR:
- Add yourself in the
contributorsdirectory as well so you show up as an author! - The app styles are slightly different (a drawer
menu instead of a header) and that's alright. We're meaning to standardize all the styles in each of the implementations but until then this is fine. - Tiny thing I noticed: story links (such as
Ask HNitems) that should route directly to the comments instead of an external source don't seem to navigate there (click anyAsk HNand you'll see what I mean). This fix doesn't have to be in this PR, it's quite minor and I'm okay with having this open as an issue if you wish to merge this in without the fix. - A couple of nitpicks with your added file
Other then that, I think this will be a good addition to HNPWA 👍
site/_apps/hn-pwa.txt
Outdated
| --- | ||
| weight: 9 | ||
| title: 'HN-PWA' | ||
| github-title: 'mozeal' |
There was a problem hiding this comment.
This should be the github title of the app, mozeal/hn-pwa, not just your username (Apologies if it's ambiguous!)
site/_apps/hn-pwa.txt
Outdated
| - name: 'Mozeal' | ||
| lighthouse: 91/100 | ||
| interactive-em: 4.817s | ||
| interactive-faster-3g: 2.696s |
There was a problem hiding this comment.
For both EMand 3G - I notice you obtained them from WPT First View times. We've been using the First Interactive (beta) times from the linked lighthouse results for each implementation so could you use that instead? (Looks like your 3G TTI is under 5s which is good 👍 )
site/_apps/hn-pwa.txt
Outdated
| lighthouse-link: https://www.webpagetest.org/lighthouse.php?test=170603_Q7_8f9c820981695c6b9fcc810c854b4362&run=3 | ||
| wpt-em-link: https://www.webpagetest.org/result/170603_Q7_8f9c820981695c6b9fcc810c854b4362/ | ||
| wpt-faster-3g-link: https://www.webpagetest.org/result/170603_8G_7c8f401cd68ccf076e1c2e00147c98a4/ | ||
| image: http://qrx.io/ss/hn-pwa.png |
There was a problem hiding this comment.
So the rest of the implementations have been using a phone frame for each of the screenshots. What I used was MockUPhone to get any phone frame and just upload the image there (you may need to compress the output as well with something like Compressor.io as the images do come out quite large)
However we do need to change things up a bit and use either a single phone frame or a generic outline (#14) but until then this will work. I can later update all the phone frames when I get the time.
site/_apps/hn-pwa.txt
Outdated
| lighthouse-link: https://www.webpagetest.org/lighthouse.php?test=170603_Q7_8f9c820981695c6b9fcc810c854b4362&run=3 | ||
| wpt-em-link: https://www.webpagetest.org/result/170603_Q7_8f9c820981695c6b9fcc810c854b4362/ | ||
| wpt-faster-3g-link: https://www.webpagetest.org/result/170603_8G_7c8f401cd68ccf076e1c2e00147c98a4/ | ||
| image: http://qrx.io/ss/hn-pwa.png |
There was a problem hiding this comment.
Another thing is that we host all the images in our site which makes for easier service worker caching. Could you just save the image in the assets/images directory to make things simpler here?
site/_apps/hn-pwa.txt
Outdated
| lighthouse-link: https://www.webpagetest.org/lighthouse.php?test=170604_0Q_306ecb0bc3f53815f01357f8fa6e1cbb&run=2 | ||
| wpt-em-link: https://www.webpagetest.org/result/170604_3H_17cf72d4b241ea2d5fae6142e860ef64/ | ||
| wpt-faster-3g-link: https://www.webpagetest.org/result/170604_0Q_306ecb0bc3f53815f01357f8fa6e1cbb/ | ||
| image: /asset/images/hn-pwa-mobile.png |
There was a problem hiding this comment.
Little typo here, needs to reference the assets folder not asset.
| hosting: 'Google AppEngine' | ||
| other-details: '' | ||
| authors: | ||
| - name: 'Mozeal' |
There was a problem hiding this comment.
So in order for the contributor to show up underneath their app, you'll need to make sure the name here matches that in the contributor submission (for example if you wanted to use the name Mozeal, you'll have to put that exact string here and in your submission under _contributors
|
Friendly ping with respect to the review comments @mozeal :) |
|
@mozeal do you think you might have time to take a look at the above feedback comments? |
|
friendly ping :) |
|
I’m so sorry for lately reply.
just re-submitted.
Best
Moz
… On Jul 16, 2560 BE, at 4:03 AM, Addy Osmani ***@***.***> wrote:
friendly ping :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#55 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFl7NOluE3IMMMX2-SxNbtErqHJYmdzks5sOSkogaJpZM4NvCKQ>.
|
|
@mozeal Could you double check the timings in your submission are using the numbers for "first interactive" from the Lighthouse reports in WebPageTest? e.g Go to your report:
Click the Lighthouse score:
Use the numbers from the performance section:
Right now for example the faster 3G and EM numbers do not appear to match those in these reports. |
|
In case he has time, I'm also looping in @anubhav7495 for his thoughts on this implem :) |
|
@mozeal The app looks a little different but that is mostly fine for now.
Please try to fix Lighthouse AuditsFaster 3G The above lighthouse audits also highlight some opportunities to improve the performance even further, please try to take a look at that too. Thanks. |
|
Friendly ping @mozeal in case you have time to work on those last remaining bits. We'd love to have this implementation land. |
|
Hey folks. As it's been a few quarters since the last updates on this PR, we're going to go ahead and close it. If we would like to explore this PR once again, please update the weights based on master and tackle the comments from @agubler in #55 (comment). Thanks once again for your efforts to contribute here! |
Submit for hacker-news-pwas