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

Porting Banner to Next [1616] #1667

Merged
merged 11 commits into from
Feb 10, 2021
Merged

Porting Banner to Next [1616] #1667

merged 11 commits into from
Feb 10, 2021

Conversation

huyxgit
Copy link

@huyxgit huyxgit commented Feb 6, 2021

Issue This PR Addresses

Fixes #1616
Fixes #1554
Fixes #1553

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@humphd
Copy link
Contributor

humphd commented Feb 6, 2021

This is getting closer, but see differences with production:

Screen Shot 2021-02-06 at 11 24 06 AM

@humphd
Copy link
Contributor

humphd commented Feb 6, 2021

Nice, getting closer. We still have a white margin at the bottom to get rid of:

Screen Shot 2021-02-06 at 1 00 35 PM

Also, the font is wrong for the main title, etc. Maybe that's part of a different PR we need? Not sure.

@huyxgit
Copy link
Author

huyxgit commented Feb 6, 2021

Nice, getting closer. We still have a white margin at the bottom to get rid of:

Screen Shot 2021-02-06 at 1 00 35 PM

Also, the font is wrong for the main title, etc. Maybe that's part of a different PR we need? Not sure.

@humphd Idk, something going on with the Header maybe.
Tried to import Roboto at global css (not a good idea since all changed to roboto)
now Idk why fontFamily: 'Roboto' in Banner.tsx isn't working

@humphd
Copy link
Contributor

humphd commented Feb 6, 2021

It looks like the image is too tall by about the same amount of height as the header adds, so when you scroll down, the image is below the viewport. Compare to master https://dev.telescope.cdot.systems/ where the image is exactly at the bottom of the viewport.

@humphd
Copy link
Contributor

humphd commented Feb 6, 2021

cc @tonyvugithub who might have thoughts on how to do these fonts.

@huyxgit
Copy link
Author

huyxgit commented Feb 7, 2021

It looks like the image is too tall by about the same amount of height as the header adds, so when you scroll down, the image is below the viewport. Compare to master https://dev.telescope.cdot.systems/ where the image is exactly at the bottom of the viewport.

I found css problem happened after this PR #1659

@huyxgit
Copy link
Author

huyxgit commented Feb 8, 2021

@foxviet : that PR is to ensure the background color applied to all pages. Nothing to do with the specific css problem that this PR is currently experiencing. I would look at the stylesheet to fix this.

I meant this one you mentioned:

also caused the 'black' in the fab scrolldown button,, not just the 'login' one on the header

src/frontend/next/src/components/Banner.tsx Outdated Show resolved Hide resolved
src/frontend/next/src/components/Banner.tsx Outdated Show resolved Hide resolved
src/frontend/next/src/components/Banner.tsx Outdated Show resolved Hide resolved
src/frontend/next/src/components/Banner.tsx Outdated Show resolved Hide resolved
src/frontend/next/src/components/Banner.tsx Outdated Show resolved Hide resolved
src/frontend/next/src/components/Banner.tsx Outdated Show resolved Hide resolved
@PedroFonsecaDEV
Copy link
Contributor

PedroFonsecaDEV commented Feb 8, 2021

@foxviet Could you please provide us a screenshot of this PR running on your end?

@huyxgit
Copy link
Author

huyxgit commented Feb 8, 2021

@foxviet Could you please provide us a screenshot of this PR running on your end?

@PedroFonsecaDEV your suggestion fixed the 'scrolldown' button to make it looks normal, but the font issue is still something else. Now it's the same as on Netlify deploy preview.

The CSS issue with 'login' button & the height of image I think it's in Header & DynamicImage components

Copy link
Contributor

@HyperTHD HyperTHD left a comment

Choose a reason for hiding this comment

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

Almost there...
Just need to fix the height of the scroll down button and we gtg

Here's what you got right now:
netlify-banner

How it should look like:
Prod-banner

@huyxgit
Copy link
Author

huyxgit commented Feb 9, 2021

Almost there...
Just need to fix the height of the scroll down button and we gtg

Here's what you got right now:
netlify-banner

How it should look like:
Prod-banner

I saw and fixed it! Thanks for your note bro

@PedroFonsecaDEV
Copy link
Contributor

Approving.
Since the Image size will be fixed on #1683

@huyxgit huyxgit merged commit 7422c55 into Seneca-CDOT:master Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: front-end area: nextjs Nextjs related issues
Projects
None yet
7 participants