Apply opinionated defaults to LogoSuite

[joshfarrant]

List of notable changes:

• Added new default variant to LogoSuite which:
  • Applies emphasis styles if there are 5 or fewer logos
  • Applied muted styles if there are 6 or more logos
• Reduced logo size on mobile
• Removed unnecessary (and technically unsupported) Fragments from LogoSuite Stories
• Tidied up LogoSuite stories
• Don't actively encourage stacking LogoSuite components in docs or Storybook
• Prefer examples with visually-hidden headings

What should reviewers focus on?

• Make sure you're happy with the regenerated snapshots
• Does the functionality of the new default variant make sense and is it communicated appropriately in the docs?

Supporting resources (related issues, external links, etc):

• https://github.com/github/primer/issues/3290

Contributor checklist:

• All new and existing CI checks pass
• Tests prove that the feature works and covers both happy and unhappy paths
• Any drop in coverage, breaking changes or regressions have been documented above
• New visual snapshots have been generated / updated for any UI changes
• All developer debugging and non-functional logging has been removed
• Related issues have been referenced in the PR description

Reviewer checklist:

• Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
• Check that tests prove the feature works and covers both happy and unhappy paths
• Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Before	After

[changeset-bot]

🦋 Changeset detected

Latest commit: bccd348

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@primer/brand-primitives	Minor
@primer/react-brand	Minor
@primer/brand-e2e	Minor
@primer/brand-fonts	Minor
@primer/brand-config	Minor
@primer/brand-storybook	Minor

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

[github-actions]

🟢 No design token changes found

[github-actions]

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

[rezrah]

Is the breaking change still valid following recent changes? If it is, could we be more specific about whether it's a visual one vs API? Guessing it's the former, in which case we should give the user a nudge about what to look for.

Example you can reference from another recent change: https://github.com/primer/brand/pull/616/files#diff-bf4a5c10ff09f2964e747a5873b4c1b2124a813202d314f69effbc3de1faf3dfR7-R8

[joshfarrant]

I've updated the changeset, let me know what you think 👍

[rezrah]

This is cool, better than what's currently here so let's go with it 👍

Leaving some additional thoughts for posterity too, so we can come back to use of Omit vs Pick.

I could be reading this wrong, but when adding a new prop to LogoSuite we unfortunately won't get it for 'free' here unless we opt in. I think we're doing this because we want to avoid a conflict in names between named children, but that's potentially an edge case. I'd assume in most cases we'd want to benefit from auto opt-in where possible. We could instead manually opt-out those conflicting props as they are really only solving the need to flatten Storybook arguments and nothing to do with the component itself.

[joshfarrant]

Yes that's exactly right. I thought this behaviour would be more predictable and intuitive.

You want to add a new control to the story? Just add the type to the top of the file too.

As opposed to

You want to add a new prop to one of the LogoSuite components? It might cause conflicts in the Storybook types, regardless of whether this prop is actually used in Storybook.

It's just a matter of preference, but I prefer the explicit, opt-in approach to the more magic, opt-out approach. Adding a type to cover new functionality feels much more natural to me than having types be automatically added for me and then potentially having to manage conflicts. If we adopt this approach everywhere then conflicts might not be that uncommon as we reuse prop names fairly regularly across components.

[rezrah]

Thanks @joshfarrant, appreciate you going through all that feedback. LogoSuite is looking really cool 👍