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

ADAPT-5902 | @rebeccahongsf | theme mega menu #695

Merged
merged 20 commits into from
Sep 1, 2023

Conversation

rebeccahongsf
Copy link
Contributor

@rebeccahongsf rebeccahongsf commented Aug 28, 2023

READY FOR REVIEW

Summary

Review By (Date)

  • Retroreview :)

Review Tasks

Setup tasks and/or behavior to test

  1. Navigate to https://deploy-preview-695--stanford-alumni.netlify.app/
  2. Verify that the mega menu appears as outlined in figma
  3. Review code

Associated Issues and/or People

@rebeccahongsf rebeccahongsf marked this pull request as ready for review August 29, 2023 03:48
<li>
<SbLink link={link}>{linkText}</SbLink>
<li className="su-rs-mb-0">
<SbLink
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question for Tuesday @yvonnetangsu 😃 — Does Storyblok link field auto-magically define the external links differently from the internal links? I know we have a conditional to check within the SbLink component but I was wondering if any additional code work is needed to make it work 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@rebeccahongsf I don't believe you need to do anything to the link prop, but I'm looking at the current navItem component, and there is a hasExternalLinkIcon prop that you can set (whether to display an external link icon) 😄
https://github.com/SU-SWS/saa_alumni/blob/dev/src/components/navigation/navItem.js

Copy link
Member

Choose a reason for hiding this comment

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

It's more for whether you want all external links to display an icon - I don't think you need to do the external link check yourself.

@yvonnetangsu
Copy link
Member

@rebeccahongsf One thing I noticed is that you might be able to reuse more of the responsive styles for the top level parent buttons for smaller breakpoints. You're probably still working on it though? 😄

PR preview:
Screenshot 2023-08-29 at 12 27 19 PM

On live site:
Screenshot 2023-08-29 at 12 19 00 PM

@rebeccahongsf
Copy link
Contributor Author

@rebeccahongsf One thing I noticed is that you might be able to reuse more of the responsive styles for the top level parent buttons for smaller breakpoints. You're probably still working on it though? 😄

PR preview: Screenshot 2023-08-29 at 12 27 19 PM

On live site: Screenshot 2023-08-29 at 12 19 00 PM

Ooo! This is a great call out, @yvonnetangsu! 😅 I totally overlooked the parent text second line field

@yvonnetangsu
Copy link
Member

Ooo! This is a great call out, @yvonnetangsu! 😅 I totally overlooked the parent text second line field

I figured you were probably still working on it too - there is no rush! 😄

@yvonnetangsu
Copy link
Member

yvonnetangsu commented Aug 30, 2023

@rebeccahongsf Very nice work - this is 95% there 😄 ! Here comes just a few comments then should be GTG

  • On LG and +, when a mega menu panel is open, please add functionality to close with the escape key
  • Please ask designer for a hocus state for this close button 😄

Screenshot 2023-08-30 at 10 05 40 AM

  • For Travel Study, there seems to be a visual regression which cause the normal state of the close button to become black, and the hocus state is taking on the one for homesite as well

Screenshot 2023-08-30 at 10 05 58 AM

Screenshot 2023-08-30 at 10 08 01 AM

  • This is a designer thing - I would push back hard on this layout especially for the XL breakpoint. It really seems like esp for the panels with the card, it could use the full width of the container here

Screenshot 2023-08-30 at 9 55 22 AM

  • Also, another thing to check with designer - since the masthead of the SAA homesite doesn't quite follow the normal center container (ie, the screen margins are a bit different to accommodate the logo treatment, I'd ask the designer whether to align the content container to this modified container, eg, on the left edge, have the first column in the panel left aligned with the left side of the logo.

  • I would suggest finetuning the width/flex treatment of the individual column a bit (maybe best to use a CSS grid here), eg, currently if I have a long item in a column it would cause the other column to shrink

Screenshot 2023-08-30 at 10 14 29 AM

There are different ways to update this, using a CSS grid might be the most straightforward, but you can also add su-shrink-0 to each column and give them each a max width etc. Up to you and the designer 😄

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Just added some comments for final finetuning - looking really great! Nice job 👏🏼 😄

@rebeccahongsf
Copy link
Contributor Author

@yvonnetangsu I updated this PR with some of your requested changes.

I'm going to follow up with a designer and implement the following changes in my next PR. Please let me know if any additional adjustments are needed. Thank you thank you! 😃

  • Please ask designer for a hocus state for this close button 😄

Screenshot 2023-08-30 at 10 05 40 AM

  • For Travel Study, there seems to be a visual regression which cause the normal state of the close button to become black, and the hocus state is taking on the one for homesite as well

Screenshot 2023-08-30 at 10 05 58 AM

Screenshot 2023-08-30 at 10 08 01 AM

  • Also, another thing to check with designer - since the masthead of the SAA homesite doesn't quite follow the normal center container (ie, the screen margins are a bit different to accommodate the logo treatment, I'd ask the designer whether to align the content container to this modified container, eg, on the left edge, have the first column in the panel left aligned with the left side of the logo.

@rebeccahongsf rebeccahongsf merged commit f028d5d into megamenu Sep 1, 2023
6 checks passed
@rebeccahongsf rebeccahongsf deleted the feature/ADAPT-5902--themeMegamenu branch September 1, 2023 17:07
@yvonnetangsu
Copy link
Member

@rebeccahongsf Thank you for the changes - I'll look after getting into SRWC 👍🏼 😄

@yvonnetangsu
Copy link
Member

Looks great - @rebeccahongsf !!

Adding that comment for grid gap as you requested - here's a screenshot - need gaps on both of the grids 👍🏼 😄
Screenshot 2023-09-01 at 1 19 17 PM

@yvonnetangsu
Copy link
Member

A few more that I just noticed - related to the modal visual regression 😂 Definitely related to the earlier comments, so you might fix these when you fix those ones.

  • Looks like the mobile user menu has some visual regression with the links/buttons
    Screenshot 2023-09-01 at 1 22 24 PM

  • Looks like the search modal Close X button is black so blending in (mobile and desktop)
    Screenshot 2023-09-01 at 1 22 35 PM

@yvonnetangsu
Copy link
Member

yvonnetangsu commented Sep 1, 2023

I'm not able to log in as zguan on the preview, if you could - please do a check for the user menu for me too 😄

@rebeccahongsf
Copy link
Contributor Author

I'm not able to log in as zguan on the preview, if you could - please do a check for the user menu for me too 😄

Looks like I was missing the MEGAPROFILE_URL in vault! Mind retrying here?
https://megamenu--stanford-alumni.netlify.app/

I'll definitely take a look over the user menu as well 😃

@yvonnetangsu
Copy link
Member

I'm not able to log in as zguan on the preview, if you could - please do a check for the user menu for me too 😄

Looks like I was missing the MEGAPROFILE_URL in vault! Mind retrying here? https://megamenu--stanford-alumni.netlify.app/

I'll definitely take a look over the user menu as well 😃

Thank you @rebeccahongsf ! Yes, I can log in now 👏🏼

Yup, looks like some visual regression in the user menu on the homesite after logging in as well
Screenshot 2023-09-05 at 12 24 12 PM

On the T/S side, it looks good except for the close modal button 😄 (for user menu, search - as mentioned before, and for the Destination page filter modal on mobile)
Screenshot 2023-09-05 at 12 25 15 PM

Screenshot 2023-09-05 at 12 27 54 PM

@rebeccahongsf
Copy link
Contributor Author

Thank you thank you @yvonnetangsu for looking it over again! Looks like my changes to the Modal.styles.ts is causing those issues so I'll fix that up in my mobile pr 🫡

@yvonnetangsu
Copy link
Member

Thank you @rebeccahongsf !! 😄 👍🏼

rebeccahongsf added a commit that referenced this pull request Sep 18, 2023
* update README

* ADAPT-5924 | @rebeccahongsf | update to megamenu vault vars

* ADAPT-5901 | @rebeccahongsf | build out mega menu components (#694)

* ADAPT-5901 | @rebeccahongsf | build out mega menu components

* remove dupe

* ADAPT-5902 | @rebeccahongsf | theme mega menu (#695)

* ADAPT-5902 | @rebeccahongsf | theme mega menu

* adjust dropdown menu flex and grid container

* tweak fonts

* tweak font and spacing

* style mega menu card image

* tweak grid to match mocks

* fixup toplink styles

* fix double menu appearance

* adjust responsive display of dropdown links

* add some mobile styles

* fixup

* fixup

* fixup

* update active state logic

* remove homesite

* adjust background color, text color, and tweak chevron buttons  of mobile menu

* fixup menu dropdown transition

* fixup keyboard esc

* add mobile background color for dropdowns

* fixup grid

* DS-173 | @rebeccahongsf | theme mobile mega menu (#696)

* revert modal styles for testing

* remove unused color

* add modal styles for mobile menu

* fixup mobile font size and chevron hocus

* add grid gap

* add mobile menu group border and spacing

* adjust mobile border and chevron

* create seperate mega menu card stylesheet

* fixup mobile positioning for child menu

* reduce padding on lg and add word break

* fixup grid

* fixup active state

* add grid gaps

* remove secondary header link

* DS-173 | @rebeccahongsf | adjust mega menu card width

* DS-173 | @rebeccahongsf | fixup line height of menu link items and add white bg for mobile menu modal

* DS-96 | @rebeccahongsf | replace div with span for accessibility

* DS-173 | @rebeccahongsf | fixup card white space

* DS-96 | @rebeccahongsf | update aria-label for mega menu

* revert netlify config and readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants