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

DS-42 DS-496 | Decanter and fonts update; small visual regression and bug fixes #710

Merged
merged 27 commits into from
Jan 5, 2024

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Sep 13, 2023

READY FOR REVIEW

Summary

Review By (Date)

  • When does this need to be reviewed by?

Review Tasks

Setup tasks and/or behavior to test

  1. Go to the preview, both homesite and T/S. Check everything on desktop and mobile
  2. Please pay attention to all the things in the masthead in particular (search, menu, user menu). For both SAA and T/S sites.
  3. See all the things I called out below
  4. Look at code
  5. @rebeccahongsf Please help me review all things that you've added - eg, membership, TS forms and anything that I missed. Thanks! 😄

Associated Issues and/or People

@yvonnetangsu yvonnetangsu changed the title WIP - DS-42 | Decanter and fonts update; small visual bug fixes WIP - DS-42 | Decanter and fonts update; small visual bug fixes; use node 18 Sep 13, 2023
@yvonnetangsu yvonnetangsu marked this pull request as draft September 13, 2023 17:59
.nvmrc Outdated Show resolved Hide resolved
@sherakama
Copy link
Member

@yvonnetangsu should this be delegated to @rebeccahongsf?

@yvonnetangsu
Copy link
Member Author

@yvonnetangsu should this be delegated to @rebeccahongsf?

There are a few things that are a bit obscured and requires some testing on my end. I'll try to get this done soon. I'll likely ask @rebeccahongsf to look at the membership and form stuff to make sure there's no visual regression though - thanks in advance! 😄

* dev: (28 commits)
  NoJira | @shc314 | Adds two Hero components in red and blue color gradients (#757)
  4.3.4
  Replace storyblok-rich-text-react-renderer with storyblok-rich-text-react-renderer-ts (#780)
  4.3.3
  Revert "DS-162: Remove Blanket 404 Redirect. (#768)"
  4.3.2
  ADVPS-12724: Restore Register Redirect (#770)
  4.3.1
  DS-162: Remove Blanket 404 Redirect. (#768)
  DS-456: upgrade node version and npm packages (#759)
  4.3.0
  DS-160 | @rebeccahongsf | build gsb membership card (#752)
  DS-312 | @rebeccahongsf | update to cypress 13 and add tests (#728)
  4.2.4
  DS-13 | @rebeccahongsf | remove mega tag containing application name (#751)
  4.2.3
  DS-413: Associates Designees redirect (#741)
  4.2.2
  DS-392 | @rebeccahongsf | limit text input to 10 char (#733)
  4.2.1
  ...

# Conflicts:
#	.nvmrc
#	package-lock.json
#	package.json
#	src/components/cards/gsbCard.js
#	src/components/partials/htmlHead.js
#	src/styles/forms.css
@yvonnetangsu yvonnetangsu changed the title WIP - DS-42 | Decanter and fonts update; small visual bug fixes; use node 18 WIP - DS-42 | Decanter and fonts update; small visual bug fixes Jan 4, 2024
Comment on lines +109 to +114
"@tailwindcss/forms": "^0.5.7",
"autoprefixer": "^10.4.16",
"axios-mock-adapter": "^1.22.0",
"cross-env": "^7.0.3",
"cypress": "^13.6.0",
"decanter": "^7.1.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

Reinstall these as dev dependencies instead of dependencies

@@ -2,24 +2,24 @@ import { dcnb } from 'cnbuilder';

export const root = ({ orientation, isDark }) =>
dcnb(
'su-group su-relative su-w-full su-overflow-hidden su-bg-saa-black su-break-words su-basefont-23 su-bg-clip-padding su-border su-border-solid su-backface-hidden',
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no longer a need to use su-border-solid in newer versions of TW. It's solid by default so only the su-border class is needed.

!isDark && orientation !== 'horizontal',
}
);
export const imageWrapper = ({ orientation }) =>
dcnb('su-relative su-overflow-hidden', {
'su-w-full su-mb-[-4em] md:su-mb-0 md:su-w-1/2 su-h-[60vw] sm:su-h-[50vw] lg:su-h-[40vw] xl:su-h-500 su-flex-shrink-0 su-h-full':
Copy link
Member Author

Choose a reason for hiding this comment

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

su-flex-shrink-0, su-flex-grow-0, su-flex-shrink, su-flex-grow have been updated in TW to remove the flex

@@ -99,7 +99,7 @@ export const GlobalFooter = () => (
</li>
<li className={styles.secondaryMenuListItem}>
<a
href="https://bulletin.stanford.edu/pages/c7vDgeOuJIfpZe8GKmW3"
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the non-discrimination link the global footer to use vanity url

Comment on lines +82 to +86
<FlexBox
className={styles.bodyMobile}
alignItems="center"
justifyContent="between"
>
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes this issue in the TS masthead
Screenshot 2024-01-05 at 12 07 07 PM

Comment on lines 45 to 50
<a
className={dcnb('su-logo', styles.logoColor({ color }), className)}
href="https://www.stanford.edu"
aria-hidden
tabIndex={-1}
>
Copy link
Member Author

Choose a reason for hiding this comment

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

This behavior was recommended for SODA for drupal core, so we do the same thing here. We have the logo and Stanford Home that links to the same page in the global footer and are next to each other, this prevents keyboard user from tabbing to the same link one after another (skip tabbing to logo link for keyboard users)

Comment on lines +82 to +88
<FlexBox
className="su-cc"
alignItems="center"
justifyContent="between"
>
<FlexBox alignItems="center" className="su-rs-py-1">
<Logo className="su-w-[15rem]" />
<Logo className="su-w-150" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes this masthead mobile for Homesite as well
Screenshot 2024-01-04 at 1 36 36 PM

Comment on lines +32 to +34
'su-group su-bg-transparent su-font-semibold hocus:su-underline su-text-m1 su-flex su-items-end su-absolute su-top-22 su-z-10',
{
'hover:su-bg-digital-red-xlight hocus:su-rounded-full':
'su-text-white hocus:su-text-white hocus:su-bg-digital-red-xlight hocus:su-rounded-full':
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes issue with disappearing mega menu close Xbutton on live (icon color is set to white unless it's focused, eg, when you first open the modal)
Screenshot 2024-01-05 at 12 11 53 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

Also fixed this issue with missing padding on the 2nd level. This is due to the Decanter change when we have nested .cc classes, the padding in the inner one goes to 0. This is a special case because even though the nested cc classes is used in a modal.
Screenshot 2024-01-05 at 12 13 05 PM

Comment on lines +40 to +43
'su-text-white hocus:su-text-digital-red-xlight':
type !== 'mega-menu' && type !== 'timeout',
'su-right-0': type === 'trip-filter',
'su-right-20': type !== 'trip-filter',
Copy link
Member Author

Choose a reason for hiding this comment

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

Trip filter padding fix

export const menuMobile = ({ navOpened }) =>
dcnb(
'su-flex su-flex-col su-absolute su-z-20 su-bg-saa-black su-shadow-xl su-bg-white su-w-full',
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove the su-bg-white class here which causes this issue. Even though this class has always been here, before the Decanter upgrade, we don't see it because the su-bg-saa-black class was rendered after the su-bg-white in the compiled CSS file.
Screenshot 2024-01-05 at 11 12 25 AM

@@ -17,11 +17,11 @@ const Associate = ({ isEnabled = true, person }) => {

return (
<li className="su-flex even:su-bg-black-10">
<div className="su-flex-1 su-w-[50%] su-py-10 su-pl-30">
<div className="su-flex-1 su-w-1/2 su-py-10 su-pl-30">
Copy link
Member Author

Choose a reason for hiding this comment

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

Going with TW defaults by using the 1/2 instead of [50%]

@@ -4,18 +4,18 @@ export const root = ({ checked }) =>
dcnb(
'su-group su-pt-3 su-py-4 su-pl-20 sm:su-pl-30 md:su-pl-50 lg:su-p-3 hover:su-bg-palo-verde-dark focus-within:su-bg-palo-verde-dark lg:su-bg-saa-black-dark su-mt-6 lg:focus-within:su-bg-gradient-to-bl lg:hover:su-bg-gradient-to-bl lg:focus-within:su-from-saa-electric-blue lg:hover:su-from-saa-electric-blue lg:focus-within:su-to-palo-verde-dark lg:hover:su-to-palo-verde-dark su-cursor-pointer',
{
'lg:su-bg-gradient-to-bl lg:su-from-saa-electric-blue su-to-palo-verde-dark':
'lg:su-bg-gradient-to-bl lg:su-from-saa-electric-blue lg:su-to-palo-verde-dark':
Copy link
Member Author

Choose a reason for hiding this comment

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

the lg: prefix is here which fixes this issue

before:
Screenshot 2024-01-05 at 12 22 41 PM

after
Screenshot 2024-01-05 at 12 23 32 PM

@yvonnetangsu yvonnetangsu marked this pull request as ready for review January 5, 2024 20:24
@yvonnetangsu
Copy link
Member Author

READY FOR REVIEW

Summary

Review By (Date)

  • When does this need to be reviewed by?

Review Tasks

Setup tasks and/or behavior to test

  1. Go to the preview, both homesite and T/S. Check everything on desktop and mobile
  2. Please pay attention to all the things in the masthead in particular (search, menu, user menu). For both SAA and T/S sites.
  3. See all the things I called out below
  4. Look at code
  5. @rebeccahongsf Please help me review all things that you've added - eg, membership, TS forms and anything that I missed. Thanks! 😄 If they're not on netlify preview, please pull down locally

Associated Issues and/or People

  • DS-42

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Code changes look good.
I did a quick bit of 'linky clicky' and things still look good to me.

@sherakama
Copy link
Member

Thanks for all the inline comments as well.

I've updated the tests to run again. Looks like they just failed. Let me look at why.

@sherakama
Copy link
Member

@yvonnetangsu on our next 1:1, lets chat about how #759 introduced the visual regression issues and what we should do differently next time. I'd like this to be at a point where we don't have to go through you to review everything but I also don't want to have to set up extensive visual regression checks.

Lets talk.

@yvonnetangsu
Copy link
Member Author

@yvonnetangsu on our next 1:1, lets chat about how #759 introduced the visual regression issues and what we should do differently next time. I'd like this to be at a point where we don't have to go through you to review everything but I also don't want to have to set up extensive visual regression checks.

Lets talk.

Thank you for the review @sherakama! Yeah I agree, if we could get an automated test for visual regression in like diffy that'd be awesome. Let's chat! 😄

Should I merge this or wait till Rebecca has a chance to review the forms and membership stuff?

@sherakama
Copy link
Member

@yvonnetangsu on our next 1:1, lets chat about how #759 introduced the visual regression issues and what we should do differently next time. I'd like this to be at a point where we don't have to go through you to review everything but I also don't want to have to set up extensive visual regression checks.
Lets talk.

Thank you for the review @sherakama! Yeah I agree, if we could get an automated test for visual regression in like diffy that'd be awesome. Let's chat! 😄

Should I merge this or wait till Rebecca has a chance to review the forms and membership stuff?

Give Rebecca a chance to chime in first as she sees stuff that I won't.

Copy link
Contributor

@rebeccahongsf rebeccahongsf left a comment

Choose a reason for hiding this comment

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

I checked the T/S and Membership and submitted some forms 😄 Everything looks as expected! We should be good to go! 🚀 Thank you @yvonnetangsu

@yvonnetangsu
Copy link
Member Author

Thank you @rebeccahongsf and @sherakama ! I'm going to merge to dev, then create a release. Deploying on Friday? Oh yeah.... 🤣

@yvonnetangsu yvonnetangsu merged commit e18be5d into dev Jan 5, 2024
6 checks passed
@yvonnetangsu yvonnetangsu deleted the feature/DS-42_update-decanter-fonts branch January 5, 2024 23:20
@sherakama
Copy link
Member

Deploying on Friday? Oh yeah.... 🤣

Our systems are stable. Go for it!

@yvonnetangsu yvonnetangsu changed the title DS-42 | Decanter and fonts update; small visual regression and bug fixes DS-42 DS-496 | Decanter and fonts update; small visual regression and bug fixes Jan 5, 2024
@yvonnetangsu yvonnetangsu mentioned this pull request Jan 5, 2024
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.

3 participants