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

[Testimonial] Site refactor updates #424

Merged
merged 9 commits into from
Sep 18, 2023

Conversation

josepmartins
Copy link
Contributor

@josepmartins josepmartins commented Sep 14, 2023

Summary

As part of the https://github.com/github/primer/issues/2494, we'll need to apply some additional change requests to the testimonial component to unblock upcoming, flagship marketing pages.

Docs: https://primer-bb88e44c6c-26139705.drafts.github.io/components/Testimonial/react
Storybook: https://primer-bb88e44c6c-26139705.drafts.github.io/storybook/?path=/story/components-testimonial--playground

List of notable changes:

  • updated provider layout styles, simplifies the layout and class options
  • removed align prop, tests and documentation
  • removed line height component tokens, as they need to be custom

What should reviewers focus on?

  • Storybook matches latest designs for large and small size variants
  • Documentation matches latest designs, align examples and props are removed

Steps to test:

  • Storybook:
  • Documentation:

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

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:

Please try to provide before and after screenshots or videos

Small variant

Before After
Screenshot 2023-09-14 at 11 51 48 Screenshot 2023-09-14 at 11 51 55

Large variant

Before After
Screenshot 2023-09-14 at 11 52 42 Screenshot 2023-09-14 at 11 52 48

@changeset-bot
Copy link

changeset-bot bot commented Sep 14, 2023

🦋 Changeset detected

Latest commit: 4a7e296

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

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Minor
@primer/brand-primitives 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
Copy link
Contributor

github-actions bot commented Sep 14, 2023

🔍 Design token changes found

View CSS variable changes
- --brand-Testimonial-quote-letterSpacing-large: var(--brand-text-letterSpacing-600);
+ --brand-Testimonial-quote-letterSpacing-large: var(--brand-text-letterSpacing-900);
- --brand-Testimonial-quote-letterSpacing-default: var(--brand-text-letterSpacing-300);
+ --brand-Testimonial-quote-letterSpacing-default: var(--brand-text-letterSpacing-400);
- --brand-Testimonial-quote-fontWeight-default: var(--brand-text-weight-300);
+ --brand-Testimonial-quote-fontWeight-default: var(--brand-heading-weight-600);
- --brand-Testimonial-quote-fontSize-default: var(--brand-text-size-300);
+ --brand-Testimonial-quote-fontSize-default: var(--brand-text-size-400);

@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2023

⚠️ 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:

artifacts section of workflow run

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

Review visual differences

@josepmartins josepmartins temporarily deployed to github-pages September 14, 2023 09:52 — with GitHub Actions Inactive
@josepmartins josepmartins marked this pull request as ready for review September 14, 2023 10:33
@josepmartins josepmartins temporarily deployed to github-pages September 14, 2023 10:59 — with GitHub Actions Inactive
@josepmartins josepmartins changed the title Testimonial updates [Testimonial] Site refactor updates Sep 14, 2023
Copy link
Collaborator

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

LGTM...

FYI the docs images are showing old styles 👇, but can be updated later..

Screenshot 2023-09-14 at 15 19 31

@josepmartins
Copy link
Contributor Author

josepmartins commented Sep 15, 2023

FYI the docs images are showing old styles 👇, but can be updated later..

Yep I'm aware, but since there's been lot of changes I think we should tackle documentation once everything is settle down.

I included Testimonial as part of the "Update the documentation" issue https://github.com/github/primer/issues/2620

@josepmartins josepmartins temporarily deployed to github-pages September 15, 2023 04:15 — with GitHub Actions Inactive
@josepmartins josepmartins temporarily deployed to github-pages September 15, 2023 04:46 — with GitHub Actions Inactive
@josepmartins josepmartins merged commit 61eba57 into main Sep 18, 2023
12 of 14 checks passed
@josepmartins josepmartins deleted the josepmartins/testimonial-remove-align branch September 18, 2023 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants