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

Editing page weight chapter #331

Merged
merged 5 commits into from
Nov 6, 2019

Conversation

foxdavidj
Copy link
Contributor

Progress on #218

@rviscomi rviscomi added the writing Related to wording and content label Nov 5, 2019
@rviscomi rviscomi added this to the SHIP IT! milestone Nov 5, 2019
@foxdavidj
Copy link
Contributor Author

@tammyeverts @khempenius @rviscomi

I've finished the editing but have a few questions and notes:

  1. I removed the mention of latency being governed by the speed of light. This is a really interesting topic to bring up, but needs to be explained further if you're going to mention it.
  2. How do you feel about putting the glossary components in a bullet list like i have?
  3. Are figure 11 and 12 for images served on mobile?
  4. Since we are have captions underneath each table, should we remove the headings File size by image format for images > 100 bytes and File size by image format for images > 1024 bytes?

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

Lots of comments. I think you can just do a batch commit for everything. A few require manual intervention.

A few high-level comments:

  • 6MB should be written as 6 MB
  • 1234% should be written as 1,234%
  • figcaptions should end with a period
  • headings should be sentence case, not title case
  • dashes are almost always not the right punctuation, prefer commas or colons as necessary
  • use ellipses sparingly (see above)

@khempenius
Copy link
Contributor

@rviscomi All the suggested changes look fine to me - however Github is saying I don't have permission to push to this repo, so you may need to do that.

@khempenius
Copy link
Contributor

Re: combining desktop & mobile tables into one table: I personally find that type of table more difficult to read, but that's just me - I'd be fine with either style.

Feedback

Co-Authored-By: Rick Viscomi <[email protected]>
@foxdavidj
Copy link
Contributor Author

@rviscomi Thanks for the edits :)

  1. I agree with @khempenius on combining tables. It makes them very difficult to read. I'm a fan of showing both pieces of data separately and then adding additional charts when keying in on specific insights.

  2. @khempenius you not being able to push changes here is due to this being from my fork of the repo. If you have extra changes you'd like to make just ping me here and i'll make them for you 👍

@rviscomi
Copy link
Member

rviscomi commented Nov 6, 2019

Ok this is good to go. Any other changes can be filed in a new PR. Thanks everyone!!

@rviscomi rviscomi merged commit cc949fa into HTTPArchive:master Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
writing Related to wording and content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants