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

feat(Hero): create multiple hero components for new DS #11132

Merged
merged 27 commits into from
Oct 25, 2023

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Sep 12, 2023

Description

Adds a new set of Hero components per DS.

  • Homepage Hero and story
  • Hub Hero and story
  • Content Hero and story
  • MDX hero and story
  • Content Hero with Big Number stats not included (to be added in a future PR)
  • New hero illustration implemented for the Home Page and Learning Hub only

Related Issue

Closes #10753

Part of Implementation of DS v1 #9548

@github-actions github-actions bot added the event 📅 This issue or pull request is related to an event listing label Sep 12, 2023
@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 12, 2023

✅ ethereum-org-website-dev deploy preview ready

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro any update with Hub hero illustrations?

@nloureiro
Copy link
Contributor

@nloureiro any update with Hub hero illustrations?

for the new illustration? we might still be one week from getting the first one.

are these new illustrations blocking you from any developments?

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Sep 12, 2023

@nloureiro not a blocker here. I was thinking of just keeping this in draft until they become available. Also keeping it in draft because there is also another primitive component from the DS still missing for this (Big Numbers component for content hero)

If you don't want to wait on the illustrations, then maybe when ready can mark the Hero components as "offline" so they are not used for prod until all assets are available?

@nloureiro
Copy link
Contributor

Oh, the big numbers. I'll be sure to look into it.

regarding the rest, I wanted to have everything ready and tested before we have the illustrations.
Can this be removed from the draft and have a developer preview to test on different browsers and resolutions?

I sense we need some design adjustments, and I wanted to do it on the browser.

Thank you for your help :)

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Sep 14, 2023

@nloureiro
Copy link
Contributor

@nloureiro I went ahead and switched to "Ready for Review" :)

https://63b7ea99632763723c7f4d6b-dgtmjvennt.chromatic.com/?path=/story/organisms-layouts-hero--home-hero

Looks great! I approve it on Chromatic, I wonder if that leads to an approval here on the PR itself.

One last thing that I wanted to test is on an actual page.
Can this be on the learning hub page on the developer preview?

Thank you

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro I went ahead and switched to "Ready for Review" :)

https://63b7ea99632763723c7f4d6b-dgtmjvennt.chromatic.com/?path=/story/organisms-layouts-hero--home-hero

Looks great! I approve it on Chromatic, I wonder if that leads to an approval here on the PR itself.

One last thing that I wanted to test is on an actual page.
Can this be on the learning hub page on the developer preview?

Thank you

@nloureiro the approval you made was through the "UI Review" check on here, which did update. "UI Tests" remains pending.

I can update the learning page using the Eth2 Illustration

@nloureiro
Copy link
Contributor

@nloureiro I went ahead and switched to "Ready for Review" :)
https://63b7ea99632763723c7f4d6b-dgtmjvennt.chromatic.com/?path=/story/organisms-layouts-hero--home-hero

Looks great! I approve it on Chromatic, I wonder if that leads to an approval here on the PR itself.
One last thing that I wanted to test is on an actual page.
Can this be on the learning hub page on the developer preview?
Thank you

@nloureiro the approval you made was through the "UI Review" check on here, which did update. "UI Tests" remains pending.

I can update the learning page using the Eth2 Illustration

got, just approve it

Yes, great, we can use the Eth2 illustrations for testing purposes. Thank you!!!

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro updated the Learning Hub Page

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Good job @TylerAPfledderer

Is this ready for review? as I see in the comments that you changed it bc you needed the preview deploy for testing purposes. You are also probably waiting for #11133 to be merged (will be soon).

└── ComponentA/
├── index.tsx
├── ComponentA.stories.tsx
└── // Any other files as applicable (utils, child components, useHook, etc.)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this autoformat was triggered when dev was merged to update the branch. Lets revert this changes as this format is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip Darnit. Every time I try to save the changes locally, it wants to fix the formatting. -_-

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I think is because it is coming as a new/modified file when you update the branch with dev (that is caught by prettier). Shouldn't happen in the upcoming PRs you make.

@TylerAPfledderer
Copy link
Contributor Author

Good job @TylerAPfledderer

Is this ready for review? as I see in the comments that you changed it bc you needed the preview deploy for testing purposes. You are also probably waiting for #11133 to be merged (will be soon).

There is GatsbyImage as well as getting the Big Numbers component online too.

I don't see the latter being a blocker if that can be added in a future PR, if currently these heroes are only to be tested in preview and Storybook and not yet deployed to prod.

@pettinarip
Copy link
Member

Good job @TylerAPfledderer
Is this ready for review? as I see in the comments that you changed it bc you needed the preview deploy for testing purposes. You are also probably waiting for #11133 to be merged (will be soon).

There is GatsbyImage as well as getting the Big Numbers component online too.

I don't see the latter being a blocker if that can be added in a future PR, if currently these heroes are only to be tested in preview and Storybook and not yet deployed to prod.

Got it. FYI #11133 is merged now.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Beautiful!

Looking great @TylerAPfledderer.

I think we should also add a components/Hero/index.ts file to export all the nested hero components, as we are doing in other places, to be consistent and have better autocomplete dx when importing.

import * as React from "react"
import Button, { IProps as ButtonProps } from "../Button"
import { MatomoEventOptions, trackCustomEvent } from "../../utils/matomo"
import { ReactNode } from "react"
Copy link
Member

Choose a reason for hiding this comment

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

import order

* let the parent handle the spacing.
*
* This change would be done when the component is applied
* to prod.
Copy link
Member

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd work this out in a separate PR, of course 😅

*/}
<Breadcrumbs slug={breadcrumbSlug} mb="0" />
<Stack spacing="6">
<Heading size="2xl">{header}</Heading>
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be an h1 as there is no other heading in this hero.


interface MdxHeroProps {
breadcrumbs: BreadcrumbsProps
headingTitle: string
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with the rest

Suggested change
headingTitle: string
title: string

import { CommonHeroProps } from "../utils"

export interface ContentHeroProps extends CommonHeroProps {
breadcrumbSlug: BreadcrumbsProps["slug"]
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent with the MdxHero api, I'd call this breadcrumb and accept any breadcrumb prop.

// and then just
<Breadcrumbs {...breadcrumbs} />

@nloureiro
Copy link
Contributor

hey @TylerAPfledderer

we have the final image for the learning hub

we can prepare the final version for realise 🚀
Learn_hero

@TylerAPfledderer
Copy link
Contributor Author

hey @TylerAPfledderer

we have the final image for the learning hub

we can prepare the final version for realise 🚀
Learn_hero

@nloureiro
To clarify, I wouldn't put the hub hero into prod because I have to account for the other hub pages with new images...

Or just keep the updated learning hub page only for prod with the new hero component and this new image?

@nloureiro
Copy link
Contributor

hey @TylerAPfledderer
we have the final image for the learning hub
we can prepare the final version for realise 🚀
Learn_hero

@nloureiro To clarify, I wouldn't put the hub hero into prod because I have to account for the other hub pages with new images...

Or just keep the updated learning hub page only for prod with the new hero component and this new image?

I'm not sure if I understand, but what I have in mind is to publish just this hero now.
Whenever we have new illustrations we will change the hero to "that" one page on a seperate PR

does it make sense?

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro got it! Will get to updating the image 🤘

@pettinarip
Copy link
Member

Also, I checked the line-height for the heading. This is a bug(?). The tokens used for the breakpoints of the 2xl heading size are implemented backwards (the larger line-height of the two used is being rendered at the smaller breakpoint instead of the larger breakpoint)

I'm going to create a separate PR to address this, because it looks like they are backwards for other heading sizes in the component theme file

Edit: See #11363

Merged

@nloureiro
Copy link
Contributor

@TylerAPfledderer did you fix the spacing bug on mobile?

Screen Shot 2023-10-18 03 01 59 PM

I think was the only blocker a few weeks ago.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

I took another look at this and everything looks good 👍🏼

Once we add that new image I think we are ready to go!

@TylerAPfledderer
Copy link
Contributor Author

@TylerAPfledderer did you fix the spacing bug on mobile?

Screen Shot 2023-10-18 03 01 59 PM

I think was the only blocker a few weeks ago.

Yep, I can go ahead and make that adjustment here!

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro @pettinarip added the newest image for the learning hub and applied the spacing beneath the hero for that page.
https://ethereumorgwebsitedev01-tylerapfleddererethereumo12683.gatsbyjs.io/en/learn/

After updating the branch with the latest dev the line-height issues should also be fixed via #11363.

Also with the spacing issue in mobile: since it also showing in current prod, I would like to know if the spacing I applied is enough; I could not find a reference to work with.

@nloureiro
Copy link
Contributor

@nloureiro @pettinarip added the newest image for the learning hub and applied the spacing beneath the hero for that page. https://ethereumorgwebsitedev01-tylerapfleddererethereumo12683.gatsbyjs.io/en/learn/

After updating the branch with the latest dev the line-height issues should also be fixed via #11363.

Also with the spacing issue in mobile: since it also showing in current prod, I would like to know if the spacing I applied is enough; I could not find a reference to work with.

@TylerAPfledderer can you change the raw image to this one
Learn final layers_1970.png.zip

@konopkja noticed that it is a bit blur(ish) on the browser, and he was right, after comparing it to the original one.

Can it be the compression code that scales down the image and makes it blurry?
The raw on the repo has ~4mb and the .wepb is ~800kb it is still is a bit heavy but acceptable bot this kind of images

Let's try with a new PNG that I exported from Photoshop with more detail on it to see if it still happens

Thank you

@TylerAPfledderer
Copy link
Contributor Author

@nloureiro I added in that updated image file. I do see an improvement.

Currently we have a graphQL query that maxes the image to 1504px wide with max quality for a constrained layout.

@nloureiro
Copy link
Contributor

@nloureiro I added in that updated image file. I do see an improvement.

Currently we have a graphQL query that maxes the image to 1504px wide with max quality for a constrained layout.

yes, indeed it looks better!
oh, I did not know about the 1504px > why this size?

and, are adopting to better resolution screens, like 2x and 3x, using this script?

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented Oct 21, 2023

@nloureiro I added in that updated image file. I do see an improvement.
Currently we have a graphQL query that maxes the image to 1504px wide with max quality for a constrained layout.

yes, indeed it looks better! oh, I did not know about the 1504px > why this size?

and, are adopting to better resolution screens, like 2x and 3x, using this script?

This was something in place before I touched anything.

1504px is the max width of the container of the page content at large screens, since the image stretches the full width of its container. Gatsby will only generate a image file no larger than 1504px from the original, as well as multiple images down a set of breakpoints so the page loads an appropriate size image based on the screen size. This keeps load times down with file sizes.

Now this will be different for a page like the home page, where the image will always span the width of the screen, meaning that the original image size is the maximum generated and loaded to the page.

Aspect ratio is maintained. I don't think it is altering the image like 3x or 2x resolution.

@netlify
Copy link

netlify bot commented Oct 24, 2023

Deploy Preview for ethereumorg ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 21ca922
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/65372782af8e6b0008881073
😎 Deploy Preview https://deploy-preview-11132--ethereumorg.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 configuration.

@github-actions github-actions bot added the content 🖋️ This involves copy additions or edits label Oct 24, 2023
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢 !!

@corwintines corwintines merged commit 722f6a2 into ethereum:dev Oct 25, 2023
7 checks passed
This was referenced Oct 25, 2023
@TylerAPfledderer TylerAPfledderer deleted the feat/hero-new-ds branch October 25, 2023 16:06
This was referenced Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits event 📅 This issue or pull request is related to an event listing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hero
5 participants