feat(redesign): Homepage Hero component#2203
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| const lang = getLangFromUrl(Astro.url); | ||
| const t = useTranslations(lang); | ||
|
|
||
| const EXPRESS_VERSION = '5.2.1'; |
There was a problem hiding this comment.
Currently we have an automation on the site to update the version. I’m not sure how to suggest it here, since it could be done with a fetch on each build or we could continue with the automation.
There was a problem hiding this comment.
Right, I can fetch the latest version from the npm registry, so we're sure it's updated on every build. I can add a fallback (that can be just for example 5.x) just in case the npm registry is not reachable. wdyt?
There was a problem hiding this comment.
Yes, I think that sounds good. I think it would be fine to use https://npm.antfu.dev/express to simplify things.
There was a problem hiding this comment.
Now that I think about it, we don’t need that many automations anymore. Astro can handle them for us, which is pretty cool. I’m just saying this for pulling in the READMEs from our middlewares.
https://github.com/withastro/docs/blob/8e216293f74f38cc64db427622de74060403d5a8/src/content.config.ts#L168
There was a problem hiding this comment.
Nice! I didn't know that. I've reviewed the content config to use the content collection loader
krzysdz
left a comment
There was a problem hiding this comment.
I was wondering what is the pause/play button supposed to do, because in my browser the (light) video did not play so it did nothing. The light video is encoded with 4:4:4 chroma subsampling, which is not supported by my browser's decoder. AVC/H264 4:4:4 is not even listed in AMD, Intel or Nvidia video decode support matrix.
Error Code: NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005)
Details: auto __cdecl mozilla::SupportChecker::AddMediaFormatChecker(const TrackInfo &)::(anonymous class)::operator()(void) const: Decoder may not have the capability to handle the requested video format with YUV444 chroma subsampling.
The video files are small, which is nice, but look very much compressed and so does the light static fallback image.
If someone blocks autoplay, then the initial button state is wrong - pause button must be clicked to play the video (later the state is synchronised).
The button, when hovered, is partially clipped by the code block in mobile view (open page with enough width to load in desktop mode and then make it smaller to force mobile mode).

@krzysdz Can you please check now and tell me if the video plays now? The pause/play button is supposed to play or pause the video on demand, I've also reviewed the style so that, in case the video fails loading for any reasons, the button is not displayed.
Ok, so in this case I can try to review video/images to find a better compromise between quality and size.
Right, thanks for reporting it. It's fixed now
As the video are not loaded on mobile for performance reasons, I've actually hidden the button on mobile. |
There was a problem hiding this comment.
For the others, there are still some things to add, as well as the banner
@krzysdz I've reviewed video/poster images to find a better compromise between size and quality. Let me know how they look to you now. TY |
| } | ||
| }); | ||
| }, | ||
| { rootMargin: '5rem' } |
There was a problem hiding this comment.
| { rootMargin: '5rem' } | |
| { rootMargin: '80px' } |
I didn't review all the code, but this caught my attention. rem is not supported by rootMargin. Please fix it. Also, you can verify it in your console.
Actually it is spec bug w3c/IntersectionObserver#308
There was a problem hiding this comment.
right, it's fixed now. thanks for catching it
| } | ||
| }); | ||
| }, | ||
| { rootMargin: '5rem' } |
There was a problem hiding this comment.
Uncaught DOMException: IntersectionObserver constructor: rootMargin must be specified in pixels or percent.
The specification says that it has to be "a string of 1-4 components, each either an absolute length or a percentage".
Because of this exception more things are broken for me (e.g. the resize listener)
|
I can confirm that now both videos can be played by my browser. The exception thrown by the |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
I spotted the issue @ShubhamOulkar, should be solved now. |
|
|
||
| // https://astro.build/config | ||
| export default defineConfig({ | ||
| site: 'https://expressjs.com', |
There was a problem hiding this comment.
I removed it by mistake. Thanks for the catch. Fixed now.
| <ul> | ||
| <li> | ||
| {t('home.reviewDesignSystemPrefix')} | ||
| <a href={`/${lang}/ds-foundations`}>{t('home.reviewDesignSystemLink')}</a> |
There was a problem hiding this comment.
I really like having that page, could we add it to the menu under the Resources section?
There was a problem hiding this comment.
I added the ds-foundations page for internal reference, but of course if you find this useful, we can add it. I leave the decision to you, let me know in case :)
@krzysdz I fixed the |
There was a problem hiding this comment.
And when the user has animations disabled, does this respect that as well?
There was a problem hiding this comment.
@bjohansebas do you mean blocking the autoplay when animation is disabled?
There was a problem hiding this comment.
Yes, it's blocked if user preference is set to reduced motion
It works now after the |
| return; | ||
| } | ||
|
|
||
| const isMobile = window.innerWidth < breakpointMd; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Do you refer to the logic to disable video on mobile right? I moved it in a function that is called on load and on resize.
| const targetSrc = getVideoSource(); | ||
|
|
||
| if (currentSrc && targetSrc && !currentSrc.endsWith(targetSrc)) { | ||
| const source = video.querySelector('source'); |
There was a problem hiding this comment.
We dont need to call DOM multiple times. https://github.com/expressjs/expressjs.com/pull/2203/changes#diff-959176fc6245a55e46051932f45e47460870ef1acf3d57f64100ee864bd4a36dR183
| const alternateVideoSrc = getCurrentTheme() === 'dark' ? videoDesktopLight : videoDesktop; | ||
| if (alternateVideoSrc) { | ||
| const link = document.createElement('link'); | ||
| link.rel = 'preload'; |
There was a problem hiding this comment.
Why not to use two simple prefetch links in the html?
<link rel="prefetch" href="/hero-background-light.mp4" as="video">
<link rel="prefetch" href="/hero-background.mp4" as="video">
Instead of dynamically adding a preload link for the alternate theme video, we might consider using static prefetch links (possibly desktop-only) for both theme variants and simply switch the
|
😅 It’s 8:41 PM, I’m hungry, and I’m just clicking anywhere at this point. That’s it for today. 👋 Bye! |






Preview
This PR implements the homepage Hero component.
What's included
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.