Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

New design launch review #6615

Merged
merged 11 commits into from
Nov 2, 2021
Merged

New design launch review #6615

merged 11 commits into from
Nov 2, 2021

Conversation

devnook
Copy link
Contributor

@devnook devnook commented Nov 1, 2021

No description provided.

@netlify
Copy link

netlify bot commented Nov 1, 2021

✔️ Deploy Preview for web-dev-staging ready!

🔨 Explore the source changes: 8193d12

🔍 Inspect the deploy log: https://app.netlify.com/sites/web-dev-staging/deploys/61815a03ceebfa0008da11e5

😎 Browse the preview: https://deploy-preview-6615--web-dev-staging.netlify.app

@chrome-devrel-review-bot
Copy link
Collaborator

chrome-devrel-review-bot commented Nov 1, 2021

Hello! This is an automated review by our custom reviewbot. It updates automatically when code or GitHub comments in this pull request are created or updated.

Requested changes

If there are any common problems with the content files you created or modified, they will be listed here.

src/site/content/en/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/en/patterns/layout/pancake-stack/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/es/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/ja/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/ko/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/pl/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/pt/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/ru/index.md

  • This file passed all of our automated Markdown audits.

src/site/content/zh/index.md

  • This file passed all of our automated Markdown audits.

@google-cla google-cla bot added the cla: yes Contributor has signed the CLA label Nov 1, 2021
@devnook devnook added the DO NOT MERGE Actively working on but experimental label Nov 1, 2021
Copy link
Collaborator

@rachelandrew rachelandrew left a comment

Choose a reason for hiding this comment

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

A few copy nits.

Also, the Learn PWA image is missing the word "PWA".

src/site/content/en/index.md Outdated Show resolved Hide resolved
src/site/content/en/index.md Outdated Show resolved Hide resolved
src/site/content/en/patterns/layout/index.md Outdated Show resolved Hide resolved
src/site/content/en/index.md Outdated Show resolved Hide resolved
src/site/content/en/measure/index.njk Outdated Show resolved Hide resolved
src/site/_data/courses/design/meta.yml Show resolved Hide resolved
src/site/content/en/learn/index.njk Outdated Show resolved Hide resolved
src/site/content/en/measure/index.njk Show resolved Hide resolved
src/site/content/es/index.md Outdated Show resolved Hide resolved
src/site/content/en/index.md Show resolved Hide resolved
@rachelandrew
Copy link
Collaborator

I couldn't see it in this PR to change but under Patterns, the subhead "Layout Patterns" has Patterns capitalized, "Core Vitals patterns", lowercase. Style guide says headings should be sentence case so we should lowercase "Layout patterns".

@MichaelSolati
Copy link
Contributor

Here's some thoughts/issues

On /measure, I had to test it on the live site cause I think the key is blocked from the netlify preview, but the results are displayed in dark mode even though my experience is set in light mode. Not a breaking issue, but a wonky design thing.

image

Learn design's logo in mobile, or with the compressed toc is very wonky. Not a breaking issue.

https://deploy-preview-6615--web-dev-staging.netlify.app/learn/design/

image

Nothing I think that should stop this from landing, just things to fix at some point.

@rachelandrew
Copy link
Collaborator

@devnook I'd like to do a copy edit of the layout patterns stuff so it meets the style guide. How am I best to do that, I could check out this branch, or am I better to do a new PR? Just wondering what is least likely to cause a git-mess.

@mihajlija
Copy link
Contributor

mihajlija commented Nov 2, 2021

  • Learn PWA course is obviously WIP but it's displayed on the homepage
  • The placeholder Example PatternSet is displayed on the homepage
  • "Case Studies" in the top navigation bar should be "Case studies"
  • Case study section still listing the oldest case studies

@mihajlija
Copy link
Contributor

Not a blocker, but I think it would be better to display all available courses in a grid on the Learn page, rather than have the horizontal scroll. Especially weird since all the other items from Collections are expanded below.

https://deploy-preview-6615--web-dev-staging.netlify.app/learn/

Screenshot 2021-11-02 at 09 58 37

@rachelandrew
Copy link
Collaborator

Not a blocker, but I think it would be better to display all available courses in a grid on the Learn page, rather than have the horizontal scroll. Especially weird since all the other items from Collections are expanded below.

I'm not a fan of the carousel either, when I first looked I was going to raise an issue asking where Learn CSS was, then realised it was in a carousel, that I hadn't realised was a carousel.

@devnook
Copy link
Contributor Author

devnook commented Nov 2, 2021

@rachelandrew let's do copy updates in separate Prs please

@rachelandrew
Copy link
Collaborator

I couldn't see it in this PR to change but under Patterns, the subhead "Layout Patterns" has Patterns capitalized, "Core Vitals patterns", lowercase. Style guide says headings should be sentence case so we should lowercase "Layout patterns".

I've dealt with this in a separate PR.

@rachelandrew
Copy link
Collaborator

Patterns: #6636

@devnook
Copy link
Contributor Author

devnook commented Nov 2, 2021

@mihajlija @rachelandrew Re: carousel - it is a part of the approved design for this launch. We are not going to change the design at this point, but we can definitely come back to this topic after cds.

@devnook devnook added $-presubmit Add label to run presubmit tests. and removed DO NOT MERGE Actively working on but experimental labels Nov 2, 2021
@github-actions github-actions bot removed the $-presubmit Add label to run presubmit tests. label Nov 2, 2021
@devnook devnook merged commit 8d6758b into main Nov 2, 2021
@devnook devnook deleted the devnook-launch-review branch November 2, 2021 18:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Contributor has signed the CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants