-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
WebAssembly 2021 chapter #2563
WebAssembly 2021 chapter #2563
Conversation
Turned it on in config and staged it here: https://20211118t191759-dot-webalmanac.uk.r.appspot.com/en/2021/webassembly (link added to first comment). @rviscomi or I can update this URL with the latest periodically. |
@RReverser is also running the site locally so it's less critical that we stage it for the author's benefit. |
Ah Ok wasn’t sure if he was as config wasn’t set. Plus was nosy myself to have a look 😁 |
Ah thanks, yeah I didn't commit the config change as I was unsure if I'm supposed to leave that to maintainers :) |
Yeah will flip it back before merging so it won't accidentally go live before we give it a round of editing. But find it best to have it on for this stage for convenience without having to remember not to commit it each time! |
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 20.9%, saving 288.83 KB.
1270 images did not require optimisation. |
Still need to address some TODOs in metadata, but content-wise this should be ready for the first round of feedback. |
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 20.9%, saving 303.42 KB.
1270 images did not require optimisation. |
A table would be a good option if the extreme percentiles are needed and orders of magnitude different. |
Do you mean a table in addition to the graph or instead of one? |
I was thinking instead of a chart. |
Added back in in 3873de2. Made the labels 14px rather than 16px and extended the height a bit more. Updated the staging link (might take a few mins for cache to update though).
Well we do use the black on inner green labels. Plus it stands out so one I always spot when doing a review. I wish sheets allowed a text outline. Green text with black outline would look OK I think. But alas it doesn't support that.
Do you wanna have a think about it @RReverser and update this branch? I'll not touch it any more until I hear back from you. |
I think I'd like to keep the charts at least for 10-90 range (visualizations are so pretty! 😅) but yeah let me get back to you tomorrow. |
Is there anything to change in text or is someone else going to review it too? |
We'll do a copy edit of the text after this is merged. I like to get the controversial things out of the way in the main PR as much as possible, so that copyedit PR is easier for you to review — rather than everything having changed cause I rearranged the figures completely or the like 😁 Hopefully should just be a quick spelling and grammar check. |
+1 to this! |
Nevermind I didn't do git pull. |
Ok I think going with just big numbers or text where appropriate is good enough after all. I've adjusted wording under a graph where we removed 0% percentile. |
FWIW I think I'm done with the graphs, please take another look and rerender where necessary. |
Images updated. This good to merge now @RReverser ? |
I think so, thanks! I could definitely use some extra eyes on text and perhaps on KB -> MB conversions (I've noticed few places where I messed it up during last go through the text) but you said we'll do text review in a separate PR? |
Yup. Let's merge this and do that separate. Thanks very much for being so patient with all my requests. but think we made it better so hopefully you'll agree it was worth it! |
I mean, I still miss my extremes, but I agree it looks more consistent and better overall 😀 Thanks for all your help. |
Makes progress on #2168
Staging URL: https://20211118t191759-dot-webalmanac.uk.r.appspot.com/en/2021/webassembly