Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

404 and Use Cases page #21

Merged
merged 6 commits into from
Mar 15, 2022
Merged

404 and Use Cases page #21

merged 6 commits into from
Mar 15, 2022

Conversation

bretthayes
Copy link
Contributor

Closes #14 - 404 and use cases page

Note: I'll have to port over changes from 5170 and 5173 once those are merged in about...

@bretthayes bretthayes requested a review from katjuell March 2, 2022 23:31
@bretthayes bretthayes self-assigned this Mar 2, 2022
@bretthayes
Copy link
Contributor Author

bretthayes commented Mar 3, 2022

This includes updates for sourcegraph/about#5173 via sourcegraph/about#5175

Copy link
Contributor

@katjuell katjuell left a comment

Choose a reason for hiding this comment

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

Things look good! A few things, though:

  • We've got a lot of errors and warnings at this point, mainly from the animated logos component (and one on the use-cases index) that we should resolve:
✖ 135 problems (72 errors, 63 warnings)
  35 errors and 0 warnings potentially fixable with the `--fix` option.
  • Static builds currently fail, so we should resolve that:
$ next build && next export
Failed to compile.

./src/components/CustomerLogosSectionAnimated.tsx:214:30
Type error: Property 'height' does not exist on type 'GlobalEventHandlers'.

212 |                 const imageRef = new Image()
213 |                 imageRef.onload = function () {
> 214 |                     if (this.height && this.width) {
    |                              ^
215 |                         let calculatedWidth = (this.width / this.height) * 50
216 |                         if (calculatedWidth > 135) calculatedWidth = 135
217 |                         setImagesWidth(prevState => (prevState += calculatedWidth + 70)) //Total width of all images
error Command failed with exit code 1.
  • I noticed that there seems to be a visual discrepancy on the use cases page on mobile. Here's local on iPhone12 Pro
    Screen Shot 2022-03-04 at 9 54 57 AM
    And prod:
    Screen Shot 2022-03-04 at 9 55 10 AM

@katjuell katjuell mentioned this pull request Mar 8, 2022
@bretthayes
Copy link
Contributor Author

@katjuell I've decided to remove the logos section temporarily since we have homepage updates underway. I don't think it's worth it to revise the current implementation since I'm anticipating a rewrite for this component as soon as this homepage updates are finalized. I've left a todo and created #35 as a follow up.

@bretthayes bretthayes requested a review from katjuell March 14, 2022 21:33
Copy link
Contributor

@katjuell katjuell left a comment

Choose a reason for hiding this comment

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

LGTM! Looks like that visual issue was resolved, so must have been the logo section?
Screen Shot 2022-03-15 at 5 11 22 PM

@bretthayes
Copy link
Contributor Author

@katjuell yeah it was the logos section 🙃

@bretthayes bretthayes merged commit ddd7886 into main Mar 15, 2022
@bretthayes bretthayes deleted the brett/pages branch March 15, 2022 21:29
@elzannewentzel elzannewentzel added the team/content-platform Content Platform Team related tickets. label Mar 22, 2022
@bretthayes bretthayes added this to the Phase 1 milestone Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team/content-platform Content Platform Team related tickets.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Port over some individual pages (chunk 1)
3 participants