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

Randomize contributors on reload #1107

Merged
merged 13 commits into from
Jul 30, 2020
Merged

Conversation

tunetheweb
Copy link
Member

Since we added a 3 hour caching way back in #608 the contributor's basically don't change when refreshing the page. Not a big deal, but given they are in a random it may be less confusing if they change to confirm they are random. So this PR adds JavaScript randomisation on reload to resolve that.

It also fixes some ES2015 syntax which threw errors in old browsers like IE11 and prevents other JS (like this randomisation!) from running. In general we try to avoid this but guessed we missed this here.

@tunetheweb tunetheweb added the development Building the Almanac tech stack label Jul 26, 2020
@tunetheweb tunetheweb added this to the 2020 Platform Development milestone Jul 26, 2020
@rviscomi
Copy link
Member

I have a general uneasiness of adding unnecessary rendering work to the client. If there's any kind of processing delay, for example if the main thread is busy or the CPU is slow, the contributors would jump jarringly. I'm not sure how valuable this feature is relative to the existing functionality. @bazzadp how badly do you want this?

@tunetheweb
Copy link
Member Author

And that's why I made it only happen on reload!

  if ((performance.getEntriesByType("navigation")[0] && performance.getEntriesByType("navigation")[0].type === "reload")
      ||
        (performance.navigation && performance.navigation.type === performance.navigation.TYPE_RELOAD)) {

The performance impact of this test is negligible IMHO. Obviously there is a cost to do the actual reordering but think that's fine if reloaded.

It's not a massive deal, but I just think it's weird to show a random order, but then not to randomise again on F5.

@rviscomi
Copy link
Member

And that's why I made it only happen on reload! The performance impact of this test is negligible IMHO. Obviously there is a cost to do the actual reordering but think that's fine if reloaded.

I understand, it's just that I think it won't be negligible for everyone and I don't think the net effect is worthwhile.

It's not a massive deal, but I just think it's weird to show a random order, but then not to randomise again on F5.

Do you feel the same about the randomly generated featured chapter content on the home page?

@tunetheweb
Copy link
Member Author

And that's why I made it only happen on reload! The performance impact of this test is negligible IMHO. Obviously there is a cost to do the actual reordering but think that's fine if reloaded.

I understand, it's just that I think it won't be negligible for everyone and I don't think the net effect is worthwhile.

You think doing the check to see if this is a reload or a normal navigation won't be negligible? Or you think the reordering won't be negligible? I'm less worried about the performance impact of the reload.

It's not a massive deal, but I just think it's weird to show a random order, but then not to randomise again on F5.

Do you feel the same about the randomly generated featured chapter content on the home page?

Yes, and fixed that in similar way in #1103 😀 - actually that came first, and then decided to do this one too!

@rviscomi
Copy link
Member

Referring only to the re-rendering of the contributor order. Some users would notice the ordering change which I think is a net negative compared to the satisfaction of seeing a new order on reload. In the spirit of the "don't show something then immediately change it" principle that I just made up.

@tunetheweb
Copy link
Member Author

tunetheweb commented Jul 28, 2020

It's a good principle that you made up - and I agree with it! 😀

I just think a reload is an explicit instruction to refresh so it won't be as unexpected then.

However, more importantly, there's an animation that triggers on each page load (even if not reload). Looks like that was broken , so added a fix for that to this PR. With that, even on my poor under-powered Mac and with CPU throttling down to 6x, the shuffling is done before the animation kicks in so there is no redraw.

@tunetheweb
Copy link
Member Author

OK had another look at this after some sleep.

It looks like it doesn't draw the contributors until all the JavaScript is executed. The last thing is does is:

contributorsGrid.setAttribute('data-rendered', true);

And, until that is set, the grid should not be viewable (though there's currently a bug in production which means it is). Looks like this is to avoid a flash of all contributors before applying the filter.

So now that opacity bug is fixed in this PR, you will never experience a "flash of original ordered contributors" (FOOOC?).

So your principal is not broken, and that argument against this PR on that basis is without merit. The argument that this may be over-the-top, needless, pernicketiness very much still stands - but wouldn't be first time I'm accused of that! 😁

So, anyway, I think we should merge this.

src/templates/base/2019/contributors.html Outdated Show resolved Hide resolved
src/templates/base/2019/contributors.html Outdated Show resolved Hide resolved
src/templates/base/2019/contributors.html Outdated Show resolved Hide resolved
@tunetheweb tunetheweb requested a review from rviscomi July 30, 2020 16:55
src/templates/base/2019/contributors.html Outdated Show resolved Hide resolved
src/templates/base/2019/contributors.html Outdated Show resolved Hide resolved
@tunetheweb tunetheweb merged commit 5a0f9b7 into main Jul 30, 2020
@tunetheweb tunetheweb deleted the random_contributors_on_reload branch July 30, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Building the Almanac tech stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants