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-47 Theme & Style local footer #1

Merged
merged 14 commits into from
Apr 24, 2024
Merged

Conversation

rebeccahongsf
Copy link
Contributor

@rebeccahongsf rebeccahongsf commented Apr 10, 2024

READY FOR REVIEW

Summary

  • theme footer

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 local footer to match the figma mock content
  3. Spin up your local: yarn dev
  4. Navigate to your local dev preview
  5. Verify that the footer matches the figma mock
image
  1. Verify that the footer call to action links and buttons match the figma mock
image
  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 12, 2024 22:51
@pookmish pookmish force-pushed the feature/SUM-47--themeFooter branch from 8325df5 to 610f2ca Compare April 18, 2024 19:44
{suLocalFootSocial &&
<ul className="list-unstyled flex gap-2">
<ul className="rs-mt-4 list-unstyled flex gap-11 items-center children:mb-0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why children:mb-0 instead of mb-0 on the <li> element? I'm not asking for any changes, just curious of your choice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this was user error 😄 I was moving some things around and added seperate <li> item so I added children:mb-0 so that all children are targeted. I eventually removed it and forgot to rework the classes. In this case, we can definitely further specify and add className="children:mb-0" to the <li>

Copy link
Member

Choose a reason for hiding this comment

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

This comment not related to whether we use children:mb-0 on ul or mb-0 on li, but *: has officially replaced children: - it's in Tailwind core now 😄 @rebeccahongsf

src/components/elements/action-link.tsx Outdated Show resolved Hide resolved
src/components/elements/button.tsx Outdated Show resolved Hide resolved
src/components/elements/button.tsx Outdated Show resolved Hide resolved
src/components/elements/button.tsx Outdated Show resolved Hide resolved
src/components/elements/icons/InstagramIcon.tsx Outdated Show resolved Hide resolved
src/components/elements/icons/LinkedInIcon.tsx Outdated Show resolved Hide resolved
src/styles/fonts.tsx Show resolved Hide resolved
@pookmish
Copy link
Contributor

Also, FYI, i rebased with 1.x so make sure to update your local to this git reset --hard origin/feature/SUM-47--themeFooter

src/components/config-pages/local-footer.tsx Outdated Show resolved Hide resolved
src/components/config-pages/local-footer.tsx Outdated Show resolved Hide resolved
src/components/config-pages/local-footer.tsx Outdated Show resolved Hide resolved
src/components/config-pages/local-footer.tsx Outdated Show resolved Hide resolved
@@ -41,7 +42,7 @@ type Props = HtmlHTMLAttributes<HTMLAnchorElement | HTMLButtonElement> & {
* Disabled button element.
*/
disabled?: boolean
}
};

export const Button = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Hover/focus colors need fixed. the text color is black on the red background. it won't pass a11y

src/components/global/page-footer.tsx Outdated Show resolved Hide resolved
src/components/elements/button.tsx Outdated Show resolved Hide resolved
@rebeccahongsf rebeccahongsf requested a review from pookmish April 23, 2024 23:14
@pookmish pookmish force-pushed the feature/SUM-47--themeFooter branch from 5f45c5b to e235df6 Compare April 24, 2024 16:56
@pookmish pookmish changed the title SUM-47 | @rebeccahongsf | theme footer SUM-47 Theme & Style local footer Apr 24, 2024
@pookmish pookmish merged commit 3750816 into 1.x Apr 24, 2024
1 check failed
@pookmish pookmish deleted the feature/SUM-47--themeFooter branch April 24, 2024 16:59
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