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

Performance 2021 chapter markdown #2434

Merged
merged 43 commits into from
Nov 15, 2021

Conversation

siakaramalegos
Copy link
Member

@siakaramalegos siakaramalegos commented Nov 4, 2021

@siakaramalegos siakaramalegos added the writing Related to wording and content label Nov 4, 2021
@siakaramalegos siakaramalegos marked this pull request as draft November 4, 2021 18:51
@rviscomi rviscomi added this to the 2021 Content Writing milestone Nov 4, 2021
@tunetheweb tunetheweb changed the title Performance chapter markdown (draft) Performance 2021 chapter markdown (draft) Nov 6, 2021
@edmondwwchan edmondwwchan marked this pull request as ready for review November 8, 2021 10:31
@tunetheweb
Copy link
Member

Put "the" back to the Core Web Vitals.  I can see how it's used but it still sounded a bit awkward to me.  However, to keep it consistent, I'm adding it back.
Copy link
Member Author

@siakaramalegos siakaramalegos left a comment

Choose a reason for hiding this comment

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

Thanks for the review! Here are a few things I noted.

src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
@siakaramalegos
Copy link
Member Author

@jzyang thanks for the review - I had a few questions. Also, I think your other commit didn't make it into the pull request?

Copy link
Contributor

@jzyang jzyang left a comment

Choose a reason for hiding this comment

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

General takeaways:

  1. I highly recommend you eliminate the "I" first person; first person is for blogs and opinion pieces and lessens the reader's trust in your work;
  2. Keep your lists consistent, if you use "were good" at the start of the list, use "were poor" at the end of it, and
  3. Of course, all these are recommendations, it's up to you if you want to apply.

Almost there. I think I was fraying a bit at the end, so I apologize if I seem a bit frazzled/curt in my comments the closer I got to the conclusion.

src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
@jzyang
Copy link
Contributor

jzyang commented Nov 13, 2021

@jzyang thanks for the review - I had a few questions. Also, I think your other commit didn't make it into the pull request?

Yeah, I did a few different things and rolled back my changes when @rviscomi noted that you really don't have to take any of my recommendations as law. So finally landed on this review format that allows you to make the decisions for your own work.

@jzyang
Copy link
Contributor

jzyang commented Nov 13, 2021

I'm confused - in this commit you added "the" in one spot and took it away in another. I've seen both usages out in the wild, but I think we should stick to one convention for the article. I vote for using "the" personally.

I kept the "the" depending on context. In most of the charts, you referred to CWV without "the". When you referred to it in the Introductions, there's only one CWV, there are no other CWV to confuse it with. The times I kept it are when you refer to CWV in context with CWVs in different years, so then it makes sense you say: This is the CWV for this year (as opposed to other years). You can keep it, but then you'll also have to do changes in your charts, and in your titles: i.e. High-Level Performance: Core Web Vitals becomes High-Level Performance: the Core Web Vitals

It's really up to you. I went by your title and chart convention and deviated when I thought was appropriate. But I might have missed or incorrectly removed a few. I tried to be consistent.

@siakaramalegos
Copy link
Member Author

Thanks @jzyang - going to review them all now. FYI, first-person is recommended when giving personal opinions of the author - see style/author guide.

Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

This is a great chapter @siakaramalegos ! Have held off going through it until now but just did a complete run through and really enjoyed it.

I also did another editorial pass, including a spelling and grammar check (the best I've seen for any chapter in last 3 years btw - very little picked up!) and a few other things you might want to consider.

I think, given that, once those are reviewed, we can remove the "unedited" tag and go straight to full merge with this PR.

Well done again to all involved in this chapter!

src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
src/content/en/2021/performance.md Show resolved Hide resolved
src/content/en/2021/performance.md Outdated Show resolved Hide resolved
@jzyang
Copy link
Contributor

jzyang commented Nov 15, 2021

Thanks @jzyang - going to review them all now. FYI, first-person is recommended when giving personal opinions of the author - see style/author guide.

OK. It's just super weird for me, personally. It just doesn't read that way for me for 80-90% of the time and then suddenly get this jarring first-person (I don't mean just in your chapter), is just glaring at me like the eye of Sauron.

@tunetheweb
Copy link
Member

Most use of “I” in my chapter got changed to “this author” in editing. Maybe that’s the better phrasing and we should update the style/author guide?

Barry's suggestions

Co-authored-by: Barry Pollard <[email protected]>
@siakaramalegos
Copy link
Member Author

@tunetheweb I think "this author" sounds pretentious like the royal we so I'd prefer not to. If I'm the author, why would I refer to myself in the third person? We still have first person plural with "we".

@siakaramalegos
Copy link
Member Author

FYI, @tunetheweb changing your contributor status to reviewer since we decided Julia is the editor (listed from the start and prefer this to be one person), plus your review was more content than editing while hers was more editing than content.

@tunetheweb tunetheweb merged commit 05fb55b into HTTPArchive:main Nov 15, 2021
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.

Performance 2021
5 participants