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

BrickHack 8 Site - FAQ Section #1252

Merged
merged 6 commits into from
Dec 20, 2021
Merged

BrickHack 8 Site - FAQ Section #1252

merged 6 commits into from
Dec 20, 2021

Conversation

sjmiller7
Copy link
Contributor

Fixes #1250

Most of the structural code is the same, just messed around with colors to make the cards present as specified by the Figma.

The section itself doesn't appear centered very well (when clicking on the FAQ nav link), but this a consequence of centering the text vertically beside the ricky to eliminate space underneath.

Ricky currently has an odd bar beside him. Going to ask Design team about removing that and the section centering on Monday (11/29).

@sjmiller7 sjmiller7 added the BH8 Ohhhh yea! label Nov 28, 2021
@sjmiller7 sjmiller7 requested a review from peterkos November 28, 2021 04:58
@sjmiller7 sjmiller7 self-assigned this Nov 28, 2021
@peterkos
Copy link
Contributor

Will give these a review in the morning ("real" morning)

Copy link
Contributor

@peterkos peterkos left a comment

Choose a reason for hiding this comment

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

Looks great!

A UX ask, and a couple smaller questions:

Card UX:
This will make the UX feel more smooth: on the BH7 site, when expanding an FAQ card, it would only push all content down, not up. In the current PR, it looks like it expands the section up and down in terms of height. Is there a way to make it behave like the BH7 site, "pinned" to the top of the section?

Design q's (I can also msg these in #design)

  • No hover effect?
  • Thoughts on adding some margin around the section left/right, or are we sticking to fullwidth style? Will probably not work to well on >16" screens)

@skyegallup skyegallup self-requested a review November 30, 2021 17:33
@skyegallup
Copy link

skyegallup commented Dec 1, 2021

This will make the UX feel more smooth: on the BH7 site, when expanding an FAQ card, it would only push all content down, not up. In the current PR, it looks like it expands the section up and down in terms of height. Is there a way to make it behave like the BH7 site, "pinned" to the top of the section?

I noticed yesterday that on SJ's laptop with Chrome/Win10, this only happened when the top of the viewport was above the start of the FAQ section. Now that I'm testing on Firefox/Ubuntu, this always happens, regardless of how far down I've scrolled. I'm not exactly sure what's causing it, but it's browser-specific, which makes me a little uneasy...

Copy link

@skyegallup skyegallup left a comment

Choose a reason for hiding this comment

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

Added a couple of nits/comments in code.

One general nit: There are a few very specific sizes where one of the card's text wraps to two lines. It's an edge case for sure, but is it possible to change the content here slightly to avoid the overflow?
image

There was also one time where the navbar wouldn't shrink down when I resized my viewport from desktop-width to mobile-width, but I can't replicate it now, so 🤷 probably just Firefox being weird.

Other than that, 👍 good divs

@sjmiller7
Copy link
Contributor Author

This will make the UX feel more smooth: on the BH7 site, when expanding an FAQ card, it would only push all content down, not up. In the current PR, it looks like it expands the section up and down in terms of height. Is there a way to make it behave like the BH7 site, "pinned" to the top of the section?

I noticed yesterday that on SJ's laptop with Chrome/Win10, this only happened when the top of the viewport was above the start of the FAQ section. Now that I'm testing on Firefox/Ubuntu, this always happens, regardless of how far down I've scrolled. I'm not exactly sure what's causing it, but it's browser-specific, which makes me a little uneasy...

I have no clue how to fix this or what in the world I did to change it in the first place

@skyegallup
Copy link

This will make the UX feel more smooth: on the BH7 site, when expanding an FAQ card, it would only push all content down, not up. In the current PR, it looks like it expands the section up and down in terms of height. Is there a way to make it behave like the BH7 site, "pinned" to the top of the section?

I noticed yesterday that on SJ's laptop with Chrome/Win10, this only happened when the top of the viewport was above the start of the FAQ section. Now that I'm testing on Firefox/Ubuntu, this always happens, regardless of how far down I've scrolled. I'm not exactly sure what's causing it, but it's browser-specific, which makes me a little uneasy...

I have no clue how to fix this or what in the world I did to change it in the first place

Also, the BH7 site works completely fine on Firefox/Ubuntu regardless of how far down I've scrolled. 🤷 🤷 🤷

@sjmiller7
Copy link
Contributor Author

This will make the UX feel more smooth: on the BH7 site, when expanding an FAQ card, it would only push all content down, not up. In the current PR, it looks like it expands the section up and down in terms of height. Is there a way to make it behave like the BH7 site, "pinned" to the top of the section?

I noticed yesterday that on SJ's laptop with Chrome/Win10, this only happened when the top of the viewport was above the start of the FAQ section. Now that I'm testing on Firefox/Ubuntu, this always happens, regardless of how far down I've scrolled. I'm not exactly sure what's causing it, but it's browser-specific, which makes me a little uneasy...

I have no clue how to fix this or what in the world I did to change it in the first place

Also, the BH7 site works completely fine on Firefox/Ubuntu regardless of how far down I've scrolled. 🤷 🤷 🤷

Update to this mess: Design says no more need to have the sections be min 100vh. I will remove that when we get back to the general pr and it should fix the bug (in theory).

@sjmiller7
Copy link
Contributor Author

Design q's (I can also msg these in #design)

No hover effect?
Thoughts on adding some margin around the section left/right, or are we sticking to fullwidth style? Will probably not work to well on >16" screens)

Forgot to ask these in the last sync, but some thoughts

  • In theory, hover is not a necessary indicator because we have the plus icon. We could, for consistency's sake, give it the same hover as the buttons if you + design really want?
  • I think this is still the same as bh7? If we need to change it we can but I though it was fine

@peterkos
Copy link
Contributor

It's similar to BH7 in terms of screen real estate, but imo could be improved by a bit of padding on each side (here, 27" screen):

Current BH7
CleanShot 2021-12-18 at 23 30 06 CleanShot 2021-12-18 at 23 30 08

Also, I think I found a reason why the FAQ keeps adjusting the height in a weird way. section is display: flex, with justify-content: center. Changing that to justify-content: flex-start fixes it :)
But that can be fixed in a future PR to get it out, just writing here for info.

peterkos
peterkos approved these changes Dec 19, 2021
@sjmiller7
Copy link
Contributor Author

I think I found a reason why the FAQ keeps adjusting the height in a weird way

As stated previously, "Design says no more need to have the sections be min 100vh. I will remove that when we get back to the general pr and it should fix the bug." The reason why it is centered right now is to prevent awkward whitespace while the paces are each min 100vh. We will be adjusting the sections back in the general pr to remove the min 100vh requirement and in doing so remove the centering

a bit of padding on each side

Messing with section padding is a general pr issue imo. Either we change padding for all sections and that code needs to be done in the general pr, or we just change it around the faq cards and not the entire section in which case design needs to verify because it would be breaking alignment.

@skyegallup
Copy link

@peterkos

Also, I think I found a reason why the FAQ keeps adjusting the height in a weird way. section is display: flex, with justify-content: center. Changing that to justify-content: flex-start fixes it :) But that can be fixed in a future PR to get it out, just writing here for info.

Yes. I discovered this at a previous design sync and brought it up with SJ and Claire, who chose to resolve it by dropping the "100vh for all sections" design requirement.


@sjmiller7

Messing with section padding is a general pr issue imo. Either we change padding for all sections and that code needs to be done in the general pr, or we just change it around the faq cards and not the entire section in which case design needs to verify because it would be breaking alignment.

I think the Figma did have more padding on all sections, including the navbar:
image
image

But as you said, this should be fixed in the general PR, not this one.

Copy link

@skyegallup skyegallup left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@peterkos
Copy link
Contributor

Gotcha! Apologies, I saw the min height but wasn't positive that was the source of the bug.
LGTM

@sjmiller7 sjmiller7 merged commit 9ca2f56 into bh-1246 Dec 20, 2021
@sjmiller7 sjmiller7 deleted the bh-1250 branch December 20, 2021 01:16
sjmiller7 added a commit that referenced this pull request Jan 2, 2022
* swapping files and start gutting bh7 copy

* more gutting of css

* style refactor

* logo update

* design fixes

* mobile navbar

* design fixes

* social cards being funky

* editing social card resolution

* adjusting social card

* favicon swap

* nav underline

* outline color fix

* remove login

* fixes

* comment fixes

* contact first attempt

* changing social links

* Revert "comment fixes"

This reverts commit 75f57c6.

* Revert "Revert "comment fixes""

This reverts commit 6c75a58.

* Revert "changing social links"

This reverts commit fef9a7b.

* Revert "contact first attempt"

This reverts commit d980e9a.

* BrickHack 8 Site - Hero Section (#1253)

* starting hero

* hero section

* swapping out loop file

* fixes

* design fixes

* spacing fix

* BrickHack 8 Site - About Section (#1254)

* inital about section

* card width

* fixes

* BrickHack 8 Site - FAQ Section (#1252)

* faq styling + ricky

* fixing mobile

* removing old padding class

* fixes

* fixing broken cards

* BrickHack 8 Site - Contact & Footer (#1256)

* contact section

* fixes

* newline eof

Co-authored-by: Skye Gallup <[email protected]>

* fixing aspect ratio code

Co-authored-by: Skye Gallup <[email protected]>

* design + js fixes

* design fixes

* trying to hide play button

* trying to hide play button 2

* trying to hide play button 3

* fixing faq ricky

* fixing mobile grayout opacity

* hiding mlh banner

* copyright date

* updating stats

* prepping for open applications

* content fixes for chris

* spelling error

Co-authored-by: Chris Baudouin, Jr. <[email protected]>

* fixing spelling error

* fixing faq padding

* hero content adjustments

* hero content addition

* centering hero on mobile

Co-authored-by: Skye Gallup <[email protected]>
Co-authored-by: Chris Baudouin, Jr. <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BH8 Ohhhh yea!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants