[Brand Refactor] MinimalFooter updates - #1429
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14d976ff-b313-4037-9297-bb6554ecfdea
🦋 Changeset detectedLatest commit: 9be256c The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3cd8d822-3861-4204-a7bb-16d62f57c85c
🔍 Design token changes foundView CSS variable changes+ --brand-footer-borderColor: var(--base-color-scale-green-6);+ --brand-footer-backToTop-borderColor-rest: var(--base-color-scale-gray-2);+ --brand-footer-backToTop-borderColor-hover: var(--base-color-scale-gray-2);+ --brand-footer-backToTop-borderColor-active: var(--base-color-scale-gray-2);+ --brand-footer-backToTop-bgColor-rest: rgba(0, 0, 0, 0.01);+ --brand-footer-backToTop-bgColor-hover: rgba(0, 0, 0, 0.1164);+ --brand-footer-backToTop-bgColor-active: rgba(0, 0, 0, 0.2104);+ --brand-footer-socialIcon-restFilter: sepia(37%) saturate(152%) hue-rotate(77deg) brightness(57%);+ --brand-footer-borderColor: var(--base-color-scale-green-3);+ --brand-footer-backToTop-borderColor-rest: var(--base-color-scale-gray-6);+ --brand-footer-backToTop-borderColor-hover: rgba(255, 255, 255, 0.12);+ --brand-footer-backToTop-borderColor-active: rgba(255, 255, 255, 0.12);+ --brand-footer-backToTop-bgColor-rest: rgba(0, 0, 0, 0.01);+ --brand-footer-backToTop-bgColor-hover: rgba(255, 255, 255, 0.16);+ --brand-footer-backToTop-bgColor-active: rgba(255, 255, 255, 0.24);+ --brand-footer-socialIcon-restFilter: saturate(73%) hue-rotate(-89deg) brightness(110%);+ --brand-footer-socialIcon-restFilter: sepia(37%) saturate(152%) hue-rotate(77deg) brightness(57%);+ --brand-footer-borderColor: var(--base-color-scale-green-6);+ --brand-footer-backToTop-bgColor-active: rgba(0, 0, 0, 0.2104);+ --brand-footer-backToTop-bgColor-hover: rgba(0, 0, 0, 0.1164);+ --brand-footer-backToTop-bgColor-rest: rgba(0, 0, 0, 0.01);+ --brand-footer-backToTop-borderColor-active: var(--base-color-scale-gray-2);+ --brand-footer-backToTop-borderColor-hover: var(--base-color-scale-gray-2);+ --brand-footer-backToTop-borderColor-rest: var(--base-color-scale-gray-2); |
🟢 Bundle size report
|
🟢 No unit test coverage changes foundAll components and hooks with tests maintain the same coverage as the main branch. |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
|
|
||
| type MinimalFooterContentProps = React.HTMLAttributes<HTMLElement> & Omit<BaseProps<HTMLElement>, 'animate'> | ||
|
|
||
| function Content({children, className, ...rest}: PropsWithChildren<MinimalFooterContentProps>) { |
There was a problem hiding this comment.
@danielguillan calling it content feels a bit too broad, generic and permissive. I'm guessing this is for edge-case content, not a general purpose slot.
For arbitrary content, we've always used props like leadingComponent or trailingComponent partly because they inherently have intrinsic friction. Giving slots a named child feels like we're encouraging this as first-class API.
I missed any API explorations here, but did you have other options you considered? E.g. centerComponent or similar.
There was a problem hiding this comment.
Agree that calling it content is generic and permissive. I don't think it's necessarily for edge-cases, but making it a slot prop feels more appropriate and cohesive. I've named it centerComponent for now but I'm open to other suggestions.
There was a problem hiding this comment.
sounds good, only alternatives to centerComponent are adjacent terms like middle, etc. Not sure those are better.
the test is going to be, can you see this prop being used in other components (where arbitrary centered content is applicable).
| .Footer__copyright, | ||
| .Footer__link-text { | ||
| font-family: var(--brand-fontStack-monospace); | ||
| font-size: var(--base-size-12); |
There was a problem hiding this comment.
Random note: We should consider adding 12px to our type scale. I think we use it more than we realize.
There was a problem hiding this comment.
Yes, definitely. I always feel bad when I use or see the --base-size-12 token for a font size.
rezrah
left a comment
There was a problem hiding this comment.
Did a quick scan and it's looking good @danielguillan. Thanks for addressing the feedback I shared earlier.
|
@danielguillan stupid question, why is the footer notes included with the footer, isn't that a separate component but we are merging them together here? 👀 |
|
@danielguillan where can i see the deployment? it seeems its failing 🥺 |
…ates on deployment.
|
@jesussandreas sorry about the missing deployment, it seems there was a hiccup with GitHub Actions. Here’s the Storybook link Regarding footnotes, they’ve been part of the MinimalFooter component since day 1, no changes in this PR. See current production docs. |
|
Thank you for that @danielguillan Design notes: Can we then have "try github, enterprise and email" on the top right
above the icons, per design.
the background of the footer being canvas/subtle instead of bg/white. the font size of the footer, i have as 12px, currently in production shows as 14px - is this because is that the smallest we can go? the logo size on desktop should be 32px by 32px. IF we need to keep the footnotes, then i think we should separate them visually. to something like this, (added on the figma file for reference)
|



Summary
Towards https://github.com/github/brand-experience/issues/223
Updates
MinimalFooterwith refreshed visual treatment, an opt-in custom content slot, and a back-to-top control.List of notable changes:
MinimalFooter.Contentfor custom controls or information.MinimalFooter.BackToTopwith localized labels, configurable scroll behavior.What should reviewers focus on?
Steps to test:
MinimalFooterPlayground story.<main>element, and respects disabled and reduced-motion behavior.Supporting resources (related issues, external links, etc):
Contributor checklist:
update snapshotslabel to the PR)Reviewer checklist:
Screenshots