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

Adding chrome-100 page #2415

Merged
merged 22 commits into from
Mar 29, 2022
Merged

Adding chrome-100 page #2415

merged 22 commits into from
Mar 29, 2022

Conversation

rutuja-google
Copy link
Contributor

Fixes #SOME_ISSUE_NUMBER

Changes proposed in this pull request:

  • New page creation for chrome-100.
  • Firebase app implementation for like functionality, prod version firebase setup yet to implement.
  • Please suggest how to remove navigation item from sidebar.
  • Working on lint type errors.

@netlify
Copy link

netlify bot commented Mar 24, 2022

Deploy Preview for developer-chrome-com ready!

Name Link
🔨 Latest commit 88728dc
🔍 Latest deploy log https://app.netlify.com/sites/developer-chrome-com/deploys/6242f7c78c9fc600085e04bf
😎 Deploy Preview https://deploy-preview-2415--developer-chrome-com.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@chrome-devrel-review-bot
Copy link
Collaborator

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.

site/en/100/index.md

  • This file passed all of our automated Markdown audits.

1 similar comment
@chrome-devrel-review-bot
Copy link
Collaborator

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.

site/en/100/index.md

  • This file passed all of our automated Markdown audits.

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

I haven't had a chance to review this entire PR yet, but I wanted to share some initial comments.

@devnook @harleenkbatra08, a comment for both of you is that there are image files checked in as part of this PR. We normally try to use our image CDN for this sort of thing (and I know that the files being checked in aren't optimized).

How should we handle images needed for this project?

firebase.json Outdated Show resolved Hide resolved
firestore.indexes.json Outdated Show resolved Hide resolved
firestore.rules Outdated Show resolved Hide resolved
functions/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/main.js Outdated Show resolved Hide resolved
site/_js/slider.js Outdated Show resolved Hide resolved
@devnook
Copy link
Contributor

devnook commented Mar 25, 2022

Re: images optimization - all images have to go through our uploader pipeline: https://developer.chrome.com/docs/handbook/how-to/add-media/
The uploader outputs html code to use image as a nunjocks shortcode ("{% Img ... %}") but you can also just use the image's URL from the generated output to embed an image in a different way.

You might need a @google account to use the uploader - in this case please work with Harleen to upload the images and get the urls for them.

@devnook
Copy link
Contributor

devnook commented Mar 25, 2022

The reason that preview is not working is because you deleted the workbox-migration.svg icon, while it is actually used by this page: https://github.com/GoogleChrome/developer.chrome.com/blob/main/site/en/docs/workbox/migration/index.md

Is this icon related in any way to the /100 project?

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

I've gone through most of the remaining files, and offered feedback.

site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/firebase.js Outdated Show resolved Hide resolved
site/_js/web-components/card-section.js Outdated Show resolved Hide resolved
site/_scss/blocks/_card-section.scss Outdated Show resolved Hide resolved
site/_scss/blocks/_card-section.scss Show resolved Hide resolved
site/_scss/globals/_colors.scss Outdated Show resolved Hide resolved
site/_scss/main.scss Outdated Show resolved Hide resolved
@jeffposnick
Copy link
Contributor

A high-level comment about the experience when viewed on a device with a mobile viewport: it's very easy to get into a situation in which there is no highlighted year on the left hand side (and also, in which the very bottom of the year is just visible under the Chrome logo, instead of fully scrolled off screen).

I don't know exactly what part of your scroll event listener is responsible for this bug, but this overall feature really feels like a job of the Intersection Observer API, which will also hopefully offer better performance that the scroll event listener.

One way or another, this animation needs to be tested and fixed on mobile viewports.

Screenshot_20220325-134331

@harleenkbatra08-zz
Copy link
Contributor

In the first line of the description on the landing page can we change "Take a stroll down..." to "Take a scroll down memory lane..."? It's a great pun that Jeffy just pointed out. :)

@harleenkbatra08-zz
Copy link
Contributor

I think the arrows on each card should also be clickable to their respective links. Can we update that universally please?

@harleenkbatra08-zz
Copy link
Contributor

A high-level comment about the experience when viewed on a device with a mobile viewport: it's very easy to get into a situation in which there is no highlighted year on the left hand side (and also, in which the very bottom of the year is just visible under the Chrome logo, instead of fully scrolled off screen).

I don't know exactly what part of your scroll event listener is responsible for this bug, but this overall feature really feels like a job of the Intersection Observer API, which will also hopefully offer better performance that the scroll event listener.

One way or another, this animation needs to be tested and fixed on mobile viewports.

Screenshot_20220325-134331

Agreed about Jeffy's "year not being highlighted throughout on mobile" comment. I think we should keep the year highlighted until the first card of the next year shows up in the viewport.

@harleenkbatra08-zz
Copy link
Contributor

On desktop, the years and cookies consent dialogue are overlapping. This would need to be fixed please.
Screenshot 2022-03-25 6 17 20 PM - Display 2

@harleenkbatra08-zz
Copy link
Contributor

Again, on desktop, when you scroll with keyboard, there's a weird jump between the years 2008 & 2009. Can you please check?

@harleenkbatra08-zz
Copy link
Contributor

For the keyboard config, couple of things:

  • When I press tab starting from the first page description, the cursor goes from "Scroll to begin" to 2008 and then continues down the years. I think we should make it such that it goes down to 2008 and then moves through the content cards within 2008, and then moves to the next year (and that year's content cards and so on)
  • Also, when you go down the years via keyboard, the page isn't scrolling up to show the last few years. I assume that problem won't be an issue if we use the above navigation path, but just please keep that in mind.

@harleenkbatra08-zz
Copy link
Contributor

On desktop, the years and cookies consent dialogue are overlapping. This would need to be fixed please. Screenshot 2022-03-25 6 17 20 PM - Display 2

Oh and it's happening on mobile as well.
WhatsApp Image 2022-03-25 at 18 40 33

@harleenkbatra08-zz
Copy link
Contributor

Also, on iPhone, I can see that the spacing on the left margin is too small when the year gets highlighted. Can we resolve that please?
WhatsApp Image 2022-03-25 at 18 49 40

@rutuja-google
Copy link
Contributor Author

The reason that preview is not working is because you deleted the workbox-migration.svg icon, while it is actually used by this page: https://github.com/GoogleChrome/developer.chrome.com/blob/main/site/en/docs/workbox/migration/index.md

Is this icon related in any way to the /100 project?

Thanks. Reverted that particular image and Preview is working now.

@neha-google
Copy link

On desktop, the years and cookies consent dialogue are overlapping. This would need to be fixed please. Screenshot 2022-03-25 6 17 20 PM - Display 2
Fixed.
Oh and it's happening on mobile as well. WhatsApp Image 2022-03-25 at 18 40 33

Fixed!

thumbnail: image/x1Los57vDga6OEMNi1dIJwZ0qvp2/1gyOFYMC1sFOBkC5csD3.png
alt: 'Chrome 100'
layout: 'layouts/chrome-100.njk'
type: landing
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add draft: true to the YAML frontmatter? We want to be able to merge this PR without /100/ going live until we're ready.

Choose a reason for hiding this comment

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

When we added this in our index.md file, our Preview link stopped working so did not add it right now.

@harleenkbatra08-zz
Copy link
Contributor

harleenkbatra08-zz commented Mar 28, 2022

Actually, really sorry about this one but can you please update this to what's written here: https://docs.google.com/document/d/1ptIDUs93120ErZBAwR9tu4WXA0VD9xI6h61puDt62C8/edit?resourcekey=0-n-YwI5nmD1gIy2GpLJAeUg#heading=h.pvx544albuy5

@devnook
Copy link
Contributor

devnook commented Mar 28, 2022

@omkar-google @neha-google Omkar, I don't think we'll launch the likes functionality in this PR. Can you please remove all firebase logic (so firebase.js, script.js, meta.njk, gitignore rules) and leave just the UI? Once we get the UI in the right shape, we can re-enable likes functionality. Otherwise we wont get this Pr checked in before deadline.

@harleenkbatra08-zz
Copy link
Contributor

Share button has lost all the metadata (it's only showing the link). Please fix.
Also, the image when you share the link isn't updating. See screenshot
Screenshot 2022-03-28 11 50 34 PM
.

@harleenkbatra08-zz
Copy link
Contributor

Make Sundar video related changes (check email to Omkar & Neha)

@harleenkbatra08-zz
Copy link
Contributor

harleenkbatra08-zz commented Mar 28, 2022

In 2017, for the Photopea card, update the description to say: "A design editing web app, built by a single developer."

@devnook
Copy link
Contributor

devnook commented Mar 29, 2022

I managed to find where the chrome-100 icon was used - in the commented out code for the side navigation. @harleenkbatra08 do we plan to use that navigation (icon in the sidebar next to home,docs,blog)? If yes, please bring back the icon and make sure the commented out code works.

@devnook
Copy link
Contributor

devnook commented Mar 29, 2022

@harleenkbatra08 confirmed we don't need a link in the sidebar - please remove commented code.

es: 'Versiones'
Copy link
Member

Choose a reason for hiding this comment

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

Drive-by:

Suggested change
es: 'Versiones'
es: 'Lanzamientos'

See, for example, https://es.wikipedia.org/wiki/Ciclo_de_vida_del_lanzamiento_de_software.

Choose a reason for hiding this comment

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

We are not supposed to make any changes in the existing content hence leaving it as it is!

@devnook
Copy link
Contributor

devnook commented Mar 29, 2022

Here are errors reported by lighthouse. Lighthouse audit can be run using Chrome Devtools > Lighthouse tab. They are easy fixes - Can you please take a look?

Screenshot 2022-03-29 at 09 15 21

Screenshot 2022-03-29 at 09 15 51

Screenshot 2022-03-29 at 09 18 30

It is an empty link in this card:
Screenshot 2022-03-29 at 09 18 20

@neha-google
Copy link

Here are errors reported by lighthouse. Lighthouse audit can be run using Chrome Devtools > Lighthouse tab. They are easy fixes - Can you please take a look?

Screenshot 2022-03-29 at 09 15 21 Screenshot 2022-03-29 at 09 15 51 Screenshot 2022-03-29 at 09 18 30

It is an empty link in this card: Screenshot 2022-03-29 at 09 18 20

Done!

@neha-google
Copy link

@harleenkbatra08 confirmed we don't need a link in the sidebar - please remove commented code.

Done.

@neha-google
Copy link

single developer."

Done

@neha-google
Copy link

(check email to Omkar & Neha)

Done

@neha-google
Copy link

Share button has lost all the metadata (it's only showing the link). Please fix. Also, the image when you share the link isn't updating. See screenshot Screenshot 2022-03-28 11 50 34 PM .

We have updated the latest thumbnail image for meta and it is reflecting on our page as well.
This is mostly the cache issue and should be fixed once the site is live once we delete the Twitter cache on live.

@neha-google
Copy link

Once we get the UI in the right shape,

Removed the Firebase logic and kept the UI as it is!

@neha-google
Copy link

Actually, really sorry about this one but can you please update this to what's written here: https://docs.google.com/document/d/1ptIDUs93120ErZBAwR9tu4WXA0VD9xI6h61puDt62C8/edit?resourcekey=0-n-YwI5nmD1gIy2GpLJAeUg#heading=h.pvx544albuy5

Done! Please verify the same.

@jeffposnick jeffposnick self-requested a review March 29, 2022 12:12
@devnook devnook merged commit 6a5b4d1 into GoogleChrome:main Mar 29, 2022
@harleenkbatra08-zz
Copy link
Contributor

harleenkbatra08-zz commented Oct 11, 2022 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants