codeforpdx / website

Official Code for PDX website
https://www.codeforpdx.org
5 stars 7 forks source link

Navbar update #60

Closed kgottfri closed 2 years ago

kgottfri commented 2 years ago

Description

Type of changes

Screen Shot 2022-03-25 at 5 16 01 PM
NickSchimek commented 2 years ago

Thanks for spotting this and getting the donate button added.

This is also removing the container on the desktop view, which is something I don't think we want to do. The desktop designs may not show the container because they didn't consider larger screens. We still do want a container even though its not visible in the mock ups.

kgottfri commented 2 years ago

@NickSchimek I want to make sure we're on the same page. right now this pr adds the donate button to the sticky nav bar for desktop views. It does not remove the sticky footer for the donate button. On mobile view it will not be visible in the nav bar and will remain a sticky footer like it is right now on the live desktop site (this is being handled in another issue). Are you saying we want to keep the sticky footer on both desktop and mobile, along with the donate in the nav bar on desktop?

katrinasperry commented 2 years ago

Hey guys, from what I remember we wanted to handle the donate button differently on desktop and mobile. On desktop, it'll be in the sticky top nav and there will be no sticky footer; on mobile it doesn't fit in the top nav, so it's only in the sticky footer. Basically we only want the button showing up once in each view. I hope that makes sense!

NickSchimek commented 2 years ago

Are you saying we want to keep the sticky footer on both desktop and mobile, along with the donate in the nav bar on desktop?

What I'm saying is that there are a lot of css changes here in this PR that I don't quite understand. For instance, the removal of the container. It's my understanding we're keeping the container otherwise the text is going to stretch accross the entire screen. Which might be okay for mobile views.

Here's a pic I took that compares this PR to the current site using my laptop. The differences will really stand out if a larger monitor is used. The container is what keeps the text from running from the far left to the far right of the screen.

This PR:

Screen Shot 2022-04-02 at 9 01 31 PM

Current site:

Screen Shot 2022-04-02 at 9 01 54 PM
kgottfri commented 2 years ago

What I'm saying is that there are a lot of css changes here in this PR that I don't quite understand. For instance, the removal of the container. It's my understanding we're keeping the container otherwise the text is going to stretch accross the entire screen. Which might be okay for mobile views.

My hope was to have each section set the width individually else we would run into this issue (screenshot below) where the background is not stretched to fit the screen. By setting the wrapper width to 100% we can alternate section background colors once in main.scss rather than individually per each section and have it fill the whole background. Then we can wrap each section individually and set margin width to 80%. In this PR, the navbar width is updated, but the rest of the content is missing its individual wrapper. I guess the issue here is that merging this update to master will throw off the rest of the live site. I didn't realize this until right now. I can remove the wrapper width adjustment from this pr and it should hopefully fix this. Let me know if there's something else here that maybe I'm missing or there's a better way to do this.

Screen Shot 2022-04-02 at 11 06 32 AM
NickSchimek commented 2 years ago

Thanks for the explanation. That makes sense.

I saw two options for building the redesign: Merge everything into a new redesign branch or incrementally update the live site.

As we discussed go ahead and get everything merged into a separate branch and we can do one final push into the main branch. Thanks!