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

SUM-46 Theme & Style alert message #2

Merged
merged 7 commits into from
Apr 24, 2024
Merged

SUM-46 Theme & Style alert message #2

merged 7 commits into from
Apr 24, 2024

Conversation

rebeccahongsf
Copy link
Contributor

@rebeccahongsf rebeccahongsf commented Apr 17, 2024

READY FOR REVIEW

Summary

  • theme alert
image

Review By (Date)

  • When possible

Urgency

  • How urgent is this? (Normal, High)

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Update your drupal local to match the figma global alert
  3. Spin up your local: yarn dev
  4. Navigate to your local dev preview
  5. Verify the alert matches the figma mock
    Note:
  • Action link will match once PR SUM-47 Theme & Style local footer #1 has been merged in
  • Background color bg-fog-light will match the header once the heading work has been implemented.
  1. Review code

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory? No config export is needed

Front End Validation

Associated Issues and/or People

@rebeccahongsf rebeccahongsf marked this pull request as ready for review April 23, 2024 02:03
@yvonnetangsu
Copy link
Member

@rebeccahongsf Will this repo get preview URLs instead of having to review locally? 🤔

src/components/config-pages/global-message.tsx Outdated Show resolved Hide resolved
src/components/config-pages/global-message.tsx Outdated Show resolved Hide resolved
src/components/config-pages/global-message.tsx Outdated Show resolved Hide resolved
{suGlobalMsgHeader && <H2>{suGlobalMsgHeader}</H2>}

<Wysiwyg html={suGlobalMsgMessage?.processed}/>
<div className='bg-fog-light'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this? it puts a background behind the alert that is different than the background behind the menu/lockup area. If so, use double quotes.

src/components/config-pages/global-message.tsx Outdated Show resolved Hide resolved
@pookmish
Copy link
Contributor

Will this repo get preview URLs instead of having to review locally? 🤔

@yvonnetangsu yeah, we'll have preview URLs once we get the client to provide billing for the Vercel environment. We only get 7 days for free since the repo is in an org space. They force us into the "Pro" plan.

@rebeccahongsf rebeccahongsf requested a review from pookmish April 23, 2024 17:29
@yvonnetangsu
Copy link
Member

yvonnetangsu commented Apr 23, 2024

Will this repo get preview URLs instead of having to review locally? 🤔

@yvonnetangsu yeah, we'll have preview URLs once we get the client to provide billing for the Vercel environment. We only get 7 days for free since the repo is in an org space. They force us into the "Pro" plan.

Fantastic! Thanks @pookmish 😄 I won't be reviewing every PR, but I was asked to do periodic front end QAs (like once a month or so).

@pookmish pookmish changed the title SUM-46 | @rebeccahongsf | theme alert message SUM-46 Theme & Style alert message Apr 24, 2024
src/components/config-pages/global-message.tsx Outdated Show resolved Hide resolved
src/components/config-pages/global-message.tsx Outdated Show resolved Hide resolved
@rebeccahongsf rebeccahongsf requested a review from pookmish April 24, 2024 20:14
@pookmish pookmish merged commit e67bb0e into 1.x Apr 24, 2024
1 check failed
@pookmish pookmish deleted the feature/SUM-46--alert branch April 24, 2024 20:16
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