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

Resource Hints 2020 edits #1664

Merged
merged 30 commits into from
Dec 9, 2020
Merged

Resource Hints 2020 edits #1664

merged 30 commits into from
Dec 9, 2020

Conversation

exterkamp
Copy link
Contributor

@exterkamp exterkamp commented Dec 7, 2020

Progress on #920 #1432 cc @Zizzamia

Editors notes:

  • Spell Check + Basics
    • it's/its
    • Oxford Commas
    • Replace Quotes
    • Acronyms
    • Write out #'s < 10
  • Markdown fixes
    • Merge paragraphs into 1 line
    • Code format technical terms
    • No inline styles
    • Format tables (N/A)
  • 1st round edits + TODOs
  • Check Headings
  • Conclusion exists
  • Check Links
    • Internal
    • Make more Internal links?
    • External
  • Check graphics and Figures Figures Guide
  • Chapter Metadata ✅
  • Maybe add new quote callouts

  • Edits, round 2
  • Final spellcheck
  • Check print layout

@exterkamp exterkamp mentioned this pull request Dec 7, 2020
22 tasks
@rviscomi rviscomi added the editing Content excellence label Dec 7, 2020
@rviscomi rviscomi added this to the 2020 Content Writing milestone Dec 7, 2020
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
content="3.87%",
classes="big-number",
sheets_gid="2039808014",
sql_file="native_lazy_loading_attrs.sql"
) }}

{# TODO - revisit this figure - Ref https://github.com/HTTPArchive/almanac.httparchive.org/pull/1587#discussion_r532292106 #}
{# TODO(authors) - revisit this figure - Ref https://github.com/HTTPArchive/almanac.httparchive.org/pull/1587#discussion_r532292106 #}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure what's the split between images vs iframe based on native_lazy_loading_attrs.sql.

@khempenius can you helps us clarify more about this data, @bazzadp was asking about it here #1587

src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
@exterkamp exterkamp requested a review from Zizzamia December 8, 2020 04:45
@exterkamp exterkamp marked this pull request as ready for review December 8, 2020 04:45
@Zizzamia Zizzamia force-pushed the resource-hints-edits branch from c952ff4 to 972c73a Compare December 8, 2020 08:39

{# TODO - revisit this sentence - Ref https://github.com/HTTPArchive/almanac.httparchive.org/pull/1587#discussion_r532291496 #}
{# TODO(authors/reviewers) - revisit this sentence - Ref https://github.com/HTTPArchive/almanac.httparchive.org/pull/1587#discussion_r532291496 #}
Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the part where we say which is an astonishing result, but @bazzadp related to your comment #1587 (comment), I am not sure at this point how to surface this information. How you think is best we share it?

@rviscomi rviscomi added the ASAP This issue is blocking progress label Dec 8, 2020
Copy link
Contributor Author

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

Things are looking good! I think we're on the home stretch!

src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
@@ -199,9 +205,10 @@ Be mindful that fonts preloaded without the `crossorigin` attribute will be fetc

When it's time to choose a resource for use with different screen sizes, reach for the `media` attribute with `preload` to optimize your media queries.

{# TODO(editors): Should we keep this shorter to prevent small amounts of scrolling? #}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This might also be a question for @Zizzamia because desktop/mobile.css may be relevant to the example.

Generally, I'm ok with code scrolling, we have an overflow style to handle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I suggest removing the scroll from here is because it is so small it's almost distracting and has little to no useful content behind the scroll:
image

Only these 2 characters are hidden.

I just find it more aesthetic to not have scrolling for something this minor.

Copy link
Member

Choose a reason for hiding this comment

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

The scroll area would depend on users' viewport size so it may be more or less. I wouldn't worry about it.

classes="big-number",
sheets_gid="2039808014",
sql_file="native_lazy_loading_attrs.sql"
) }}

{# TODO - revisit this figure - Ref https://github.com/HTTPArchive/almanac.httparchive.org/pull/1587#discussion_r532292106 #}
{# TODO(authors/reviewers) - revisit this figure - Ref https://github.com/HTTPArchive/almanac.httparchive.org/pull/1587#discussion_r532292106 #}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking more over #1587 (comment) and I am not super sure if the query we run does make a distinction between iframe or img. Maybe @rviscomi or @khempenius can help a bit more with it. cc @bazzadp

src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved

The largest use is with script elements, which is unsurprising as the number of JS primary and third-party files continues to grow.

{# TODO(authors): Make sure the query link is correct, and that the gid is correct. #}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The gid is correct (1098063134), and the I think this is the right query https://github.com/HTTPArchive/almanac.httparchive.org/blob/main/sql/2020/21_Resource_Hints/priority_hints_by_importance.sql as mentioned in the figure. Right @khempenius @rviscomi ?

src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
caption="Adoption of resource hints",
description="TODO",
caption="Adoption of resource hints.",
description="A bar chart of the rate of resource hint adoption broken down by hint type and form factor. For desktop, 33% of pages use the `dns-prefetch` resource hint, 18% use `preload`, 8% use `preconnect`, 3% use `prefetch`, and less than 1% use `prerender`. Mobile is similar, except that `dns-prefetch` has 34% usage (1% higher), and `preconnect` has 9% (1% higher).",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @Zizzamia I should have linked you to the "description" guidance. I reworded them, make sure you think they are appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I seeeee! Thank you for taking care of it.

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.

LGTM. We can remove the unedited flag when this is merged.

If we don't hear from @Zizzamia tonight, are there any changes that you'd be uncomfortable merging without his OK? If so let's consider changing them to TODOs and merging the rest.

Great job and thanks for the thorough edit!

src/content/en/2020/resource-hints.md Outdated Show resolved Hide resolved
@exterkamp
Copy link
Contributor Author

This is the only comment that I am unsure of and might need a more thorough look.

#1664 (comment)

@Zizzamia
Copy link
Contributor

Zizzamia commented Dec 9, 2020

Alright, love the edits! Thank you @exterkamp and @rviscomi, I guess time to merge and deploy to prod 🔥🚀🌕

Copy link
Contributor

@Zizzamia Zizzamia left a comment

Choose a reason for hiding this comment

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

🌲🚀🌕

@exterkamp
Copy link
Contributor Author

@Zizzamia this figure looks like it is using a serif'd typeface in print view:
image

(src/static/images/2020/resource-hints/mobile-as-attribute-values-by-year.png)

@exterkamp exterkamp requested a review from Zizzamia December 9, 2020 08:36
@tunetheweb
Copy link
Member

@Zizzamia this figure looks like it is using a serif'd typeface in print view:
(src/static/images/2020/resource-hints/mobile-as-attribute-values-by-year.png)

Think I did these so let me know when you're done and I'll fix this (and then just go ahead and merge this after ready for launch!)

@tunetheweb tunetheweb merged commit ba53db9 into main Dec 9, 2020
@tunetheweb tunetheweb deleted the resource-hints-edits branch December 9, 2020 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASAP This issue is blocking progress editing Content excellence
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants