primer / brand Goto Github PK
View Code? Open in Web Editor NEWReact components and Primitives for GitHub marketing websites
Home Page: https://primer.style/brand
License: MIT License
React components and Primitives for GitHub marketing websites
Home Page: https://primer.style/brand
License: MIT License
The Button has a problem with box-shadows in focus outlines.
Example:
Steps to reproduce:
Hey folks ๐, one major use case for us is using Primer Brand components with data coming from a CMS. That works smoothly for several components but presents some challenges with a few of them. For example, the River.Content
component has a strict validation in the kind of children it may contain (according to the docs, it does only support Text
, Heading
, and Link
). That prevents us from using components such as ReactMarkdown
to parse incoming Markdown data and transform it into the same Text
and Link
components that the River.Content
component expects to contain.
Here is a link to a Codepen showing this behavior. As shown there, the ReactMarkdown
component will return a Text
element with an InlineLink
inside, something that River.Content
should be able to handle. Instead, it ignores the ReactMarkdown
element entirely and shows a blank space.
Do you have any guidance about how to proceed in such cases?
https://codepen.io/sergioalvz/full/bGmaqBx
The `River.Content` component renders the result of evaluating the `ReactMarkdown` element.
No response
No response
No response
The compete page on resources hub utilized custom River components. Those components have been switched out for React Brand in this PR. I noted some pain points in the process and creating this issue as a result. The primary paint point involved needing to override this css targeting the image:
.River__visual img,
.River__visual video {
width: 100%;
height: 100%;
object-fit: cover;
}
width: 100%
, etc.'selector-max-type': 0,
'selector-no-qualifying-type': true,
!important
Potential Remedies
className
prop:River.Content
& River.Visual
should accept a className
prop. This can be addressed in a PR for #42 if you agree.className
prop, only Text
, Heading
and Link
are allowed in River.Content
, so there isn't an option to include an internal wrapper to target (e.g. wrapper Header
and Text
in div
).align
propalign="left"
<River.Content>...</River.Content>
<River.Visual>...</River.Visual>
align="right"
<River.Visual>...</River.Visual>
<River.Content>...</River.Content>
center
prop.As described in https://github.com/github/accessibility-audits/issues/5252
Users who rely on screen readers will get impacted, if the visual name and aria-label are different for a control. The users will not get the exact information about the control and may miss the functionality.
The className
prop, as I understand it, was added so that a consumer can safely assume that every component will take it. I feel as though we should also always allow:
ref
(React Ref)id
(string)document.querySelector
I'm happy to create a PR to add a base type. Something like:
type BaseComponent = {
className?: string
id?: string
ref?: RefObject<T>
}
This came up in https://github.com/github/githubuniverse.com/pull/1761#issuecomment-1649321470.
I played around with runOnce
+ visibilityOptions
, but that doesn't seem to really help. Interestingly, if I remove all the other <AnimationProvider>
on the page it works fine. ๐ค
Maybe there is something that confuses the animationTrigger
since the animation is in the hero (above the fold) and is not "scrolled into view". And option might be to add a animationTrigger='on-load'
for animations that should just play as soon as the page loads (regardless of body scroll)?
See https://github.com/github/githubuniverse.com/pull/1761#issuecomment-1649321470
Chrome
Mac
Some of the pages we build on resources.github.com are meant for campaigns only. Ideally, these pages minimize links to focus visitors on single CTAs. Could we make children for https://primer.style/brand/components/SubdomainNavBar optional, so that we can render it with just the GitHub logo and subdomain name?
This is not related to an issue with brand but an issue in the README.
At the top of the README there is a link called primer.style/brand this link doesn't point to primer.style/brand but to primer.github.io/brand (this way works).
After there is a second link called primer.style/brand which point to primer style/brand (right) but primer.style/brand return a 404 error
Go to primer.style/brand and you will get a 404 error
Go to primer.github.io/brand and it will work
Go to primer.style/brand and get the doc like on primer.github.io/brand
There are inconsistencies with the way Mona Sans is rendered in Primer Brand and in our CDN-hosted file.
Dotcom uses a CDN to deliver Mona Sans, and is likely using the most up-to-date version of the font files.
Replace local version of Primer Brand with CDN hosted version. E.g. https://github.githubassets.com/static/fonts/github/mona-sans.woff2
Should render the same across all versions.
E.g.
No response
No response
Does this issue end up in Primer backlog inbox?
The API for Hero
, River
and FAQ
components is very different at the moment. We should strive for a common way of doing things.
It's a mix of this.
<Component heading="..." />
<Component>
<Heading>...</Heading>
</Component>
<Component>
<Component.Heading>...</Component.Heading>
</Component>
<Hero
heading="This is my super sweet hero heading"
description="Lorem ipsum dolor sit amet, consectetur adipiscing elit. In sapien sit ullamcorper id. Aliquam luctus sed turpis felis nam pulvinar risus elementum."
primaryAction={{
text: 'Primary action',
href: '#',
}}
secondaryAction={{
text: 'Secondary action',
href: '#',
}}
align="center"
/>
They can be composed in any order, but their rendered output will always be in the in the predetermined order.
This API suggests to me that I could pass in more than one <Text />
component (or other), but actually only the first one is accepted. If we control the allowed number of components and the order, there is no reason to declare River.Content
.
<River>
<River.Visual>
<img
src="https://via.placeholder.com/600x400/f5f5f5/f5f5f5.png"
alt="placeholder, blank area with an off-white background color"
/>
</River.Visual>
<River.Content>
<Heading>Heading</Heading>
<Text>
Lorem ipsum dolor sit amet, consectetur adipiscing elit. In sapien sit
ullamcorper id. Aliquam luctus sed turpis felis nam pulvinar risus
elementum.
</Text>
<Link href="#">Call to action</Link>
</River.Content>
</River>
<FAQ>
<FAQ.Heading>Frequently asked questions</FAQ.Heading>
<FAQ.Item>
<FAQ.Question>
What's included in the GitHub for Startups offer?
</FAQ.Question>
<FAQ.Answer>
<p>
All GitHub for Startups companies receive up to 20 seats of GitHub
Enterprise for free for year one and 50% off year two. Learn more
about the features and capabilities of GitHub Enterprise{' '}
<InlineLink
href="https://www.github.com"
target="_blank"
rel="noreferrer"
>
here
</InlineLink>
.
</p>
</FAQ.Answer>
</FAQ.Item>
</FAQ>
React Brand components rely on design tokens to store and transfer most design decisions and logic to the UI.
Tokens are generated internally to this repo, and don't benefit from guardrails present in Primer Primitives.
Given the importance of the design tokens and the need for stability in our end-user output, we need to safeguard accidental changes to them.
One way to do this is to surface changes to tokens more promptly and easily..
Primer Primitives currently achieves this through CI automation scripts. Example below.
To take this idea further, we could also use visual regression testing alongside code diffing as accompanying documentation for reviewers, which is more visually engaging and easier to reason about. This can be done through Storybook fixtures.
Opening the floor to other ideas on how we can safeguard against accidental / incorrect changes to our tokens.
This came up in https://github.com/github/githubuniverse.com/issues/1711#issuecomment-1539633063. The request would be to add an instagram
option to the socialLinks
and link to http://instagram.com/github.
Hey folks ๐,
I see there are some inconsistencies in components about horizontal spacing. That's especially noticeable when looking at components in narrow screen sizes. For example, the SectionIntro
component uses a padding-block
property that makes it have some horizontal space in narrow screens, while the River
or the FAQ
, for example, will be touching the screen's borders.
Right now, I'm applying some custom padding for both River
and FAQ
to create a consistent experience, but I think it would be great if we could make all components behave the same.
CTABanner doesn't currently forward the className
prop. This makes it harder to apply custom styling during integration.
Line 56 in CTABanner.tsx will need to manually pass a a destructured className as the final argument.
1. Go to the docs page https://primer.style/brand/components/CTABanner
2. Pass the `className` prop to the CTABanner.
3. Open inspector and verify that class doesn't appear on the root element
Class should be forwarded and appended to the end of all other existing class names
![primer docs snippet showing test class not being rendered into dom](https://user-images.githubusercontent.com/13340707/217572388-c1a9c8eb-0e4a-4876-ae27-aa2cf7b28062.png)
No response
No response
primer.style/brand documentation currently shows a handful of cherry-picked UI patterns, rather than the entire inventory of components available through the npm package.
List of undocumented components at time of issue creation:
The following snippet (see "required" attribute):
<FormControl required>
<FormControl.Label>Checkbox 1</FormControl.Label>
<Checkbox name="day" value="" />
</FormControl>
<FormControl>
<FormControl.Label>Checkbox 2</FormControl.Label>
<Checkbox name="night" value="" />
</FormControl>
Doesn't indicate if the checkbox is required:
^ Above ^
* is added to the checkbox title when it's required. Similar to the other fields here: https://primer.style/brand/storybook/?path=/story/components-forms-examples--git-hub-enterprise
^ Above ^
Chrome
Mac
I propose adding a "toggle-able" visual layer to our development environments. This layer would display the Primer grid columns as an overlay overtop our markup, similar to how Figma allows us to toggle the grid overlay inside of our designs.
This would utilize our tokens and would be a visual aide in development.
Often, particularly when grids are nested, it's easy to veer from the design without realizing it. A margin or padding in a parent element may affect the alignment of an element within the intended grid.
I believe having a visual indicator inside our design system would:
Note: This is not really urgent, but still thought it might be a "nice to have" at some point, since I assume we will run into it for every site/page we'll build.
This came up in https://github.com/github/accessibility-audits/issues/5220, where we got asked to add a "Skip To Content" button that is shown when a user uses the tab
key to navigate and wants to skip having to go through all the options of the nav (SubdomainNavBar
).
Here is the implementation for Universe. The button could be the first item in the SubdomainNavBar
, but not sure about adding an id="start-of-content"
that the button links to. For Universe we added it on the following <main>
element. But ideally that should happen automatically without the need to remember it. Hmm.. Maybe since it's essentially just a "skip the nav", we might can also add an empty <div id="start-of-content"
at the very end of SubdomainNavBar
? ๐ค
The current API for FormControl
relies on the id
field to assign the necessary name
attribute for input tags. I would expect to be able to assign a custom name
attribute to any input, regardless of what id
was defined for the FormControl
.
id
and name
<FormControl>
<FormControl.Label>Name</FormControl.Label>
<TextInput />
</FormControl>
id
and name
, ignores the custom name
set on the input<FormControl>
<FormControl.Label>Name</FormControl.Label>
<TextInput name="name" />
</FormControl>
id
and name
to the value name
<FormControl id="name">
<FormControl.Label>Name</FormControl.Label>
<TextInput />
</FormControl>
This came up in the Universe 2023 audit:
Users navigating in resize mode will get confused as the focus order is illogical as in forward navigation hamburger remains expanded even when focus moves out of it.
As far as I understood it..
esc
key.Note: The report mentions "200% zoom", but I think that's unrelated and only needed to make the hamburger menu appear. Alternative is to just resize the browser window.
The following snippet:
<FormControl required>
<FormControl.Label>Example</FormControl.Label>
<Textarea />
</FormControl>
Creates a textarea with *
appended to the label, so it appears required. However, it's not required to submit the form.
"required" attribute is missing from the html.
The user is unable to submit the form unless the required textarea has a value.
^ above ^
Chrome
Mac
River.Content does not accept a Heading with as="h2". River automatically applies as="h3" by default.
Using River
and FAQ
automatically uses h3
headings. Semantically, that doesn't always make sense, e.g. on https://accelerator.github.com the heading structure now goes from h1
to h3
. Would be great to be able to customize the heading to h2
etc. when needed.
Actually, I would expect any component that uses Heading
to be able to accept any parameters that Heading
accepts.
cc @aguevara23
The Heading
component currently accepts an as
prop with valid values of h1
, h2
, h3
, h4
, h5
and h6
.
The same values are then applied to the rendered output. E.g. as="h2"
will render a <h2>
.
Default styling is provided for each heading size, based on the the type scale for headings. E.g. h2
maps to 800
on the type scale.
The tight-coupling of semantic tags to text sizing creates ergonomic challenges consumer side, and is too rigid for app-side use.
Where granular control is often required around heading sizing, the only option available to escape the defaults are to pass a custom CSS class.
E.g. in #100, where example layouts have been provided in Storybook but incorrect as
sizing is used to meet the visual needs of the examples.
Add escape hatches to the Heading API, in the form of a size prop. This would override the default sizing and allow more granular control over the in-browser visuals.
E.g.
<Heading as="h2" size="4">A h2 sized to look like a h4</Heading>
This API would avoid a breaking change by maintaining the existing guardrails and system defaults. It would however, offer a sufficient escape-hatch for app developers that continues to align with our type scale.
Example:
Steps to reproduce:
We are unintentionally publishing Component.visual.spec.d.ts
files in the published packages. These aren't relevant to our users. We should consider removing them, particularly as they require manual maintenance in the monolith.
E.g. https://unpkg.com/browse/@primer/[email protected]/lib/Accordion/
When using the Grid component, some combinations of column spans are not working when set with a responsive map.
This works:
<Grid enableOverlay>
<Grid.Column span={9}>
...
</Grid.Column>
<Grid.Column span={3}>
...
</Grid.Column>
</Grid>
This does not work:
<Grid enableOverlay>
<Grid.Column span={{medium: 9}}>
...
</Grid.Column>
<Grid.Column span={{medium: 3}}>
...
</Grid.Column>
</Grid>
When using a responsive map to set the columns spans, the columns should stack as expected.
No response
Chrome
Mac
It would be useful to have a full-width prop to the Button component.
As described in https://github.com/github/accessibility-audits/issues/5262
Users with low vision who find it difficult to read the text in small font uses enlarged text with re-size, will be impacted as at 400% zoom and users will be confused if the focus move to hidden element while using keyboard tab key.
Components that may also serve as links (such as Button
or InlineLink
) have text-decoration:none
rules only applied to their normal state. This can create conflicts when sites use a stylesheet that explicitly sets text-decoration:underline
for a:hover
(for example, for accessibility reasons).
I think adding text-decoration:none
to the :hover
state would fix the problem.
Next.js encourage using the Link
component declaratively to wrap any <a>
tags.
E.g.
<Link href="/about">
<a>About Us</a>
</Link>
The Hero doesn't use a composable API, which makes composition of Next.js link impossible.
<Hero
heading="..."
description="..."
primaryAction={{
text: 'Primary action',
href: '#',
}}
secondaryAction={{
text: 'Secondary action',
href: '#',
}}
align="..."
/>
Rewrite the API for consistency with other composable API's.
<Hero align="...">
<Hero.Heading></Hero.Heading>
<Hero.Description></Hero.Description>
<Hero.Description></Hero.Description>
<Link href="#">
<Hero.PrimaryAction></Hero.PrimaryAction>
</Link>
<Link href="#">
<Hero.SecondaryAction></Hero.SecondaryAction>
</Link>
</Hero>
The following snippet:
<FormControl validationStatus='error'>
<FormControl.Label>Day</FormControl.Label>
<Checkbox name="day" value="" />
<FormControl.Validation className="test">Please select at least one day</FormControl.Validation>
</FormControl>
Produces this:
This happens because of row-reverse
flex layout method.
SubdomainNavBar isn't forwarding the css class attribute value correctly, as it's overriding the internal classes due to spreading of props.
Relevant line:
Should be fixed by destructuring the className
from rest
and composing into the existing className value.
The Button component in the recent 0.12.1
release has a visual regression, whereby the focus outline animation is transitioning very slowly into position.
Pressing the Button should not produce a transition animation for the focus outline
Reproducible in Chrome
No response
Under certain circumstances, the AnchorNav
renders only the arrow and not the section title.
See the following example:
1. Go to https://primer.style/brand/storybook/?path=/story/components-anchornav-features--fewer-anchor-links&args=offset:500
2. Make sure the offset is, at least, `500px`
3. Use a narrow screen size
4. Scroll slowly until seeing the `AnchorNav` component
5. See error
1. Go to https://primer.style/brand/storybook/?path=/story/components-anchornav-features--fewer-anchor-links&args=offset:500
2. Make sure the offset is, at least, `500px`
3. Use a narrow screen size
4. Scroll slowly until seeing the `AnchorNav` component
5. The `AnchorNav` shows the first section title
See video attached above.
No response
No response
There was a recent accessibility audit for https://githubuniverse.com.
Expected result
On the footer section, the tooltips should be defined for the twitter, github, linkedin, youtube and facebook controls.
The MinimalFooter
component currently has the visually-hidden
text for screen readers. But I assume we would also need something for mouse and keyboard users that might not know what the icon (e.g. twitter bird) means. In the audit it says adding a "tooltip", but not sure if it needs to be a Tooltip like we have in Primer React? It's still marked as "Not reviewed for accessibility". Or if adding a title
attribute that the browser shows as a tooltip would be enough? Maybe something to get clarity with the #accessibility
team first.
Expected: CTA arrow should animate on hover
Current: No animation on hover
Example page: https://resources.github.com/devops/tools/compare/
the unhovered arrow starts with the classes Primer_Brand__ExpandableArrow-module__ExpandableArrow___rkfek
, and on hover has the classes Primer_Brand__ExpandableArrow-module__ExpandableArrow___rkfek Primer_Brand__ExpandableArrow-module__ExpandableArrow--expanded___gajDr
, but looks like there's no corresponding ExpandableArrow--expanded
class in the css bundle.
go to https://resources.github.com/devops/tools/compare, hover over the start a free trial CTA.
Expected: CTA arrow should animate on hover
https://user-images.githubusercontent.com/20496719/195380869-d07bed09-5ba1-4429-93ee-4bcea0a959f1.mp4
Chrome
Mac
On newer M1 chips, we are seeing a compatibility error generating visual regression testing snapshots.
Visual regression testing snapshots are currently generated through Playwright using their official Docker image. This lets us create snapshots either on the server or locally on macbooks.
On Intel-based devices like macbooks, the image executes correctly.
From @JoshBowdenConcepts...
++ arch
+ [[ aarch64 == \a\a\r\c\h\6\4 ]]
+ echo 'ERROR: not supported on Linux Arm64'
+ exit 1
ERROR: not supported on Linux Arm64
Failed to install browsers
Error: Failed to install chrome
npm ERR! Lifecycle script `test:visual:update-snapshots` failed with error:
npm ERR! Error: command failed
npm ERR! in workspace: @primer/[email protected]
npm ERR! at location: /Users/jbowden/Documents/brand/packages/e2e
Running in emulation mode by passing --platform linux/amd64
also doesn't work.
> **Warning**
> Only reproducible on M1 Apple chips
1. Clone repo
2. Checkout `main`
3. `cd packages/e2e && npm run test:visual:update-snapshots`
4. See error
1. Clone repo
2. Checkout `main`
3. `cd packages/e2e && npm run test:visual:update-snapshots`
4. Passes without error
n/a (see main description)
No response
Mac
Add a FAQ
component variant that supports a left-aligned horizontally stacked heading.
FAQ component headings use h4
elements, which aren't currently customisable.
The inability to change this becomes problematic when h4
is not appropriate during integration.
AXE will also report a violation of heading order, which is one of the consequences of not being able to customise this value.
{
"id": "heading-order",
"impact": "moderate",
"tags": [
"cat.semantics",
"best-practice"
],
"description": "Ensures the order of headings is semantically correct",
"help": "Heading levels should only increase by one",
"helpUrl": "https://dequeuniversity.com/rules/axe/4.6/heading-order?application=axeAPI",
"nodes": [
{
"any": [
{
"id": "heading-order",
"data": null,
"relatedNodes": [],
"impact": "moderate",
"message": "Heading order invalid"
}
],
"all": [],
"none": [],
"impact": "moderate",
"html": "<h4 class=\"Primer_Brand__Heading-module__Heading___IVpmp Primer_Brand__Heading-module__Heading--4___C9jDG Primer_Brand__Accordion-module__Accordion__summary-heading_____snF\">What is GitHub Galaxy?</h4>",
"target": [
"details:nth-child(1) > summary > .Primer_Brand__Accordion-module__Accordion__summary-heading_____snF.Primer_Brand__Heading-module__Heading--4___C9jDG"
],
"failureSummary": "Fix any of the following:\n Heading order invalid"
}
]
}
Solution:
This should be customisable using the as
prop as we do in other components, with h4
becoming a default value to avoid risk of regressions.
When using MinimalFooter.Link
as a
and button
together, the links aren't aligned correctly. In the example below the last link is rendered as button
.
Use `MinimalFooter.Link` as `a` and `button` together.
All links in the footer should be aligned properly.
No response
No response
We're looking to replace the custom Footer we have on some pages of https://resources.github.com/ with MinimalFooter
. To do this, we need the option to allow button
as alternative to MinimalFooter.Link
.
Example: as you can see at the bottom of https://resources.github.com/enterprise/trial/, we have a button in the footer for "Manage cookies", which opens an overlay to manage cookie settings. The styling should be the same as the text link.
With Mona Sans, we can now offer granular control over typographic visual appearance.
E.g.
Heading
support some 'presets' or official variants at a system / library level?I would like to set my own logo in SubdomainNavBar component, but there is no setting for that.
I noticed there is a `logoHref` prop on SubdomainNavBar component but no way to change the logo.
If I can change the url that logo of SubdomainNavBar component is pointing to, I'd expect it to also support changing the logo itself.
![DESCRIPTION](LINK.png)
No response
No response
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.