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: thanking contributor (#2632) #2635

Merged
merged 10 commits into from Jan 31, 2020

Conversation

@KeziahMoselle
Copy link
Contributor

KeziahMoselle commented Oct 1, 2019

This PR aims to add a card which thank random Node.js contributors.

To Do

  • Contributor card UI
    • Show loading spinner
  • Fetch 1 random contributor
  • Optimize / Errors handling / Refactor
    • Handle requests errors
    • Use IntersectionObserver if available
    • Cache the response of the first request in localStorage and update it every month
  • Use s=80 for the avatar image
  • Remove the period before the contributions (maybe wrap it in parens)
  • Maybe use textContent instead of innerText?
  • Move .hidden to a general utility class
  • Fix IE >= 10 support
  • Fix IE <= 10 styling

Preview

preview

Fixes #2632

Live preview: https://amazing-volhard-25db2f.netlify.com/en/

@KeziahMoselle

This comment has been minimized.

Copy link
Contributor Author

KeziahMoselle commented Oct 1, 2019

Update :
preview

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Oct 1, 2019

As I recently learned the hard way, the CSS is currently used by other nodejs sites.

So, this shouldn't land until that's solved.

Also, I'm not very keen of using non standard attributes even if Chrome has the biggest market share.

layouts/partials/footer.hbs Outdated Show resolved Hide resolved
@KeziahMoselle

This comment has been minimized.

Copy link
Contributor Author

KeziahMoselle commented Oct 1, 2019

Thank you, i'll change that tomorrow !

layouts/partials/footer.hbs Outdated Show resolved Hide resolved
layouts/partials/footer.hbs Outdated Show resolved Hide resolved
layouts/partials/footer.hbs Outdated Show resolved Hide resolved
@phillipj

This comment has been minimized.

Copy link
Member

phillipj commented Oct 3, 2019

Woohoow this is cool, looking forward to seeing this in action 💯

@Trott Trott mentioned this pull request Oct 3, 2019
@XhmikosR

This comment was marked as outdated.

Copy link
Member

XhmikosR commented Oct 4, 2019

FYI that's not a proper rebase

@KeziahMoselle

This comment was marked as outdated.

Copy link
Contributor Author

KeziahMoselle commented Oct 4, 2019

Oops, I'm sorry it's my first rebase ever, what should i've done ?

@XhmikosR

This comment was marked as outdated.

Copy link
Member

XhmikosR commented Oct 4, 2019

Assuming you have the upstream repo as upstream, git rebase -i upstream/master

@KeziahMoselle

This comment was marked as outdated.

Copy link
Contributor Author

KeziahMoselle commented Oct 4, 2019

Ok thank's ! I did it without the interactive flag

@KeziahMoselle

This comment has been minimized.

Copy link
Contributor Author

KeziahMoselle commented Oct 4, 2019

Update (after HTML cleanup) :
preview

@XhmikosR

This comment was marked as resolved.

Copy link
Member

XhmikosR commented Oct 4, 2019

This is still not properly rebased.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Oct 4, 2019

Also do note height: 0 is bad. IMO you should occupy the exact dimensions to avoid reflows.

@XhmikosR

This comment was marked as outdated.

Copy link
Member

XhmikosR commented Oct 4, 2019

Here is the branch with just the current patch master...XhmikosR:thanks-contrib

@XhmikosR

This comment was marked as outdated.

Copy link
Member

XhmikosR commented Oct 4, 2019

Alright, I pushed a few more tweaks. Live preview https://pedantic-mclean-3d4d4b.netlify.com/en/

Still needs polishing, and most importantly see if this will break https://benchmarking.nodejs.org/ or https://coverage.nodejs.org/ which from a quick look it doesn't seem so.

@KeziahMoselle I suggest that you overwrite your branch with mine and keep working on that.

@KeziahMoselle KeziahMoselle force-pushed the KeziahForks:feat-thanking-contributors branch from 5b41608 to c20cdde Oct 4, 2019
@KeziahMoselle

This comment was marked as outdated.

Copy link
Contributor Author

KeziahMoselle commented Oct 4, 2019

Thank you for your help !

@MaledongGit MaledongGit requested a review from XhmikosR Oct 9, 2019
Copy link
Contributor

MaledongGit left a comment

Nice feature!!! +100

Update :
preview

I tend to use this because I think it will balance the layout with a random image, however if I use something like
image, which makes me feel making head to the wall because the left side is too crawded. That's only my opition ;)

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Oct 9, 2019

This needs to wait until we have PR previews. And see if we can make only one API request instead of 2, see if we should cache this or not and so on.

Currently this slows down the page a lot that I'm personally not sure if we should move with this.

@phillipj

This comment has been minimized.

Copy link
Member

phillipj commented Oct 9, 2019

Currently this slows down the page a lot that I'm personally not sure if we should move with this.

How do you see this slowing down the page a lot?

To me it looks like the JavaScript introduced here is mostly asynchronous, in other words not blocking rendering. And the Intersection usage supported by near 90% of users even tries to avoid triggering much of the added code until the <footer> is getting close to the viewport. Therefore I've convinced myself it shouldn't have much impact on slowing down the rendering or execution of the other parts of the website.

But you might be considering something that hasn't caught my eye yet 🤷‍♂

@KeziahMoselle

This comment has been minimized.

Copy link
Contributor Author

KeziahMoselle commented Oct 9, 2019

Adding the intersection observer was the idea of @XhmikosR, the home page is slowed down a lot by these 2 requests even with the IO :/

Should I cache the response of the first request to localStorage if supported ? If so how long ?

@phillipj

This comment has been minimized.

Copy link
Member

phillipj commented Oct 9, 2019

the home page is slowed down a lot by these 2 requests even with the IO

This makes me really curious.. Could you describe concretely what you mean by "slowing down" and "a lot"? E.g. is it the actual rendering of the entire webpage we're talking about here? How are you measuring before / after times?

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Oct 9, 2019

This makes 2 XHR requests, and also fetches the avatars.

All this doesn't come for free :) An example https://gtmetrix.com/compare/OUjbjkwL/BXCUZpol

Note that the Netlify preview doesn't have the IO patch.

@phillipj

This comment has been minimized.

Copy link
Member

phillipj commented Oct 9, 2019

Yeay, great to have some insights to base discussions around 👍

Personally think the fact that it takes more time to wait for the browser to finish all its potential HTTP requests (+ followed by some code execution) is less important, and surely not a surprise as you're hinting at.

What is actually important is how quickly visitors gets the website rendered onto their screen. The fact that some parts gets rendered later or even not at all because of network failures etc, may or may not be a problem, it completely depends on what gets rendered later. What Google says about User-centric Performance is spot on IMO:

Web pages almost always have parts that are more important than others. If the most important parts of a page load quickly, the user may not even notice if the rest of the page doesn't.

In this particular scenario we're talking about nice-to-have content in the website's footer. Surely not critical for the visitor to do what they came to the website to do. The rendering of the main content will surely not be postponed significantly because of the changes added here.

Chrome's Performance-part of Devtools shows a few important stages related to User-centric Performance metrics that's really valuable. This is the results I got when measuring the nodejs.org vs these changes on netlify:

nodejs.org

Screenshot 2019-10-09 at 13 43 01

netlify

Screenshot 2019-10-09 at 13 42 14

Bottom line; I don't think the changes purposed in this PR will negatively affect nodejs.org visitors.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 16, 2020

Just remember to not squash the patches.

Why not? What's the motivation? Is it merely to prefer authorship? If so, I'd prefer we squash and add co-author metadata. We don't need 20+ commits here. Doing so might break git bisect for some cases. If the commits were logically separated into independent changes, not squashing would make sense. But they're not.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Jan 16, 2020

Is it merely to prefer authorship

This part is not a trivial one. It's quite important. So, yeah, I do want to keep them separate. I couldn't squash more patches because there are conflicts, and this PR is the result of a lot of work.

Either way, we really need that build PR to be done so that we untie the main repo from the other ones.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 16, 2020

Is it merely to prefer authorship

This part is not a trivial one.

Didn't mean to suggest it was trivial, just that there were other mechanisms for it if that was the only concern. Sorry if "merely" came off the wrong way there!

Maybe we could do a compromise by rebasing and squashing adjacent commits by the same author into each other? So it's only 7 commits instead of 21?

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Jan 16, 2020

Maybe we could do a compromise by rebasing and squashing adjacent commits by the same author into each other? So it's only 7 commits instead of 21?

That I can try for sure.

But see if we can get someone to help us move with the build PR.

KeziahMoselle and others added 7 commits Oct 4, 2019
* return early
* hide the block by default
* remove function used only once
* remove `height: 0`
* add dns-prefetch for api.github.com
* fix: check for the card instead of the avatar
* feat: add intersection observer support
* feat: localstorage caching
  - set 2 keys : max_contributors and fetch_date
  - if max_contributors is set : refetch max contributors
  - if fetch_date is 1 month old : refetch max contributors
  - if max_contributors is set and fetch_date is less that one month old : fetch only the contributor
* feat: style contributor card, avatar and contributions links
* feat: loading spinner
* feat: handle requests error by removing the card
Also, move `.hidden` to utils.
@XhmikosR XhmikosR force-pushed the KeziahForks:feat-thanking-contributors branch from b88d72d to edbae36 Jan 20, 2020
@KeziahMoselle

This comment has been minimized.

Copy link
Contributor Author

KeziahMoselle commented Jan 20, 2020

Thank you for your help @MartijnCuppens and @XhmikosR !
I'm sorry if I commited errors that you needed to fix 🙏

Copy link
Contributor

MaledongGit left a comment

Can this be merged?

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Jan 25, 2020

@MaledongGit no. Please don't bother with this PR. I will merge it when we are ready.

@XhmikosR XhmikosR removed the on hold label Jan 31, 2020
@XhmikosR XhmikosR merged commit cb4ad74 into nodejs:master Jan 31, 2020
1 check passed
1 check passed
test
Details
@XhmikosR XhmikosR deleted the KeziahForks:feat-thanking-contributors branch Jan 31, 2020
@KeziahMoselle

This comment has been minimized.

Copy link
Contributor Author

KeziahMoselle commented Jan 31, 2020

Thank you so much for merging this PR !
A big thanks to @XhmikosR who helped a lot on this 🙏
Thank's to all the reviewers and the Node team, keep it up !
This merging made my day !

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Feb 2, 2020

Thanks for sticking with the patch till the end :)

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Feb 25, 2020

If so, I'd prefer we squash and add co-author metadata.

👍


This breaks if going to a 404 URL, because the language picker function throws, and this code never runs, leaving the user with a spinner and placeholder text.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Feb 25, 2020

@tniessen can you share the stacktrace please? Because when developing locally I don't get the 404 page.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Feb 25, 2020

BTW ideally you should make a new issue.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Feb 25, 2020

NVM, I saw the error on the live site. It's unrelated to this PR so please make a new issue. It has to do with the language toggler.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 25, 2020

Issue for toggler code failure: #2984

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

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.