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: Banner image placeholder and fallback background #3190

Merged
merged 2 commits into from
Mar 19, 2022

Conversation

DukeManh
Copy link
Contributor

@DukeManh DukeManh commented Mar 12, 2022

Issue This PR Addresses

Fixes #2925

  • Add loading banner image background by displaying a tiny 40px wide image (minimal loading time) and transitioning in the main 2000px wide image (long loading time) when it finishes loading
  • Add fallback gradient banner background
  • Fix inconsistent image src, request a specific photo versus a random image chosen by the backend

Test

  • Pull this change locally
  • Run server:
   cd src/api/image && pnpm dev

   // in another terminal
   cp config/env.staging .env

   // change IMAGE_URL in .env
   # Image Service URL
   IMAGE_URL=http://localhost:4444

  // Run the frontend
  pnpm dev
  • to see that a gradient background is shown first, followed by a placeholder image, and then the high resolution image.
  • If you are not able to see the difference, your internet is probably too fast, good. Go to the Network tab, click disable cache, and choose one of the throttling options.

Screenshot

2022-03-12.15-15-44.mp4

@gitpod-io
Copy link

gitpod-io bot commented Mar 12, 2022

@DukeManh DukeManh changed the title Feat: Banner image placeholder and fallback background feat: Banner image placeholder and fallback background Mar 12, 2022
@DukeManh DukeManh requested a review from humphd March 12, 2022 20:44
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

So cool, I love it. A few things I notice.

src/api/image/src/routes/image.js Show resolved Hide resolved
src/api/image/src/routes/image.js Outdated Show resolved Hide resolved
src/web/src/components/BannerDynamicItems.tsx Outdated Show resolved Hide resolved
@DukeManh
Copy link
Contributor Author

@humphd, I'm running this again and the images fail to load. Do you know what this error is?

image

@humphd
Copy link
Contributor

humphd commented Mar 13, 2022

I'm not 100% sure. Is that Firefox? Do you have a Service Worker on localhost:4444 that's interfering somehow (try changing port)?

@DukeManh
Copy link
Contributor Author

I'm not 100% sure. Is that Firefox? Do you have a Service Worker on localhost:4444 that's interfering somehow (try changing port)?

It's on any browser I try.

Looks like something was changed on master recently that make the image service have CORS issue. I rebased and have the issue.

I filed #3212

@DukeManh
Copy link
Contributor Author

This is currently blocked by #3212, which makes it hard to test.

@DukeManh DukeManh requested a review from humphd March 15, 2022 14:13
humphd
humphd previously approved these changes Mar 16, 2022
};

// Define a series of sizes, and let the browser figure out which one to use
function createSrcset(imageSrc: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice touch.

@RC-Lee
Copy link
Contributor

RC-Lee commented Mar 16, 2022

I'm wondering if we might need to come to a consensus if we're ignoring Eslint tests until #3228 has been merged.
Do we approve if all other tests pass

@humphd
Copy link
Contributor

humphd commented Mar 17, 2022

@rclee91 I think that's fine, and we can fix things that we catch when the checks are working properly.

RC-Lee
RC-Lee previously approved these changes Mar 17, 2022
@DukeManh
Copy link
Contributor Author

DukeManh commented Mar 17, 2022

It's interesting that ESLint recommends Number.isNaN instead of isNaN, but they behave a bit differently. https://stackoverflow.com/questions/33164725/confusion-between-isnan-and-number-isnan-in-javascript

humphd
humphd previously approved these changes Mar 18, 2022
@DukeManh DukeManh merged commit 8fa25db into Seneca-CDOT:master Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve landing page performance
3 participants