Skip to content

Conversation

joshfarrant
Copy link
Contributor

@joshfarrant joshfarrant commented Jul 25, 2024

Summary

Nudged the icon up 2px

What should reviewers focus on?

  1. Does it look better?

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

Before After

image

image

image

image

Copy link

changeset-bot bot commented Jul 25, 2024

🦋 Changeset detected

Latest commit: 4577fce

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 Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

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

Copy link
Contributor

github-actions bot commented Jul 25, 2024

🟢 No design token changes found

Copy link
Contributor

github-actions bot commented Jul 25, 2024

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@joshfarrant joshfarrant marked this pull request as ready for review July 25, 2024 13:19
@joshfarrant joshfarrant requested a review from rezrah July 25, 2024 13:19
@rezrah
Copy link
Collaborator

rezrah commented Jul 25, 2024

Looks a lot better now, thanks @joshfarrant.

The success variant is a little off... so is there anything more that can be done to improve alignment? No worries if there isn't, this is still better than what we have now.

Frame 1

@joshfarrant
Copy link
Contributor Author

Thanks @rezrah. I'll modify the alignment on a per-icon basis and nudge the check slightly higher

@joshfarrant
Copy link
Contributor Author

That's updated, good spot @rezrah.

How did you view it with the grid lines out of interest?

@rezrah
Copy link
Collaborator

rezrah commented Jul 26, 2024

That's updated, good spot @rezrah.

How did you view it with the grid lines out of interest?

There's a toolbar item in Storybook for enabling a grid. It's not perfect, but helpful in situations like this.

Screenshot 2024-07-26 at 10 24 46

@rezrah
Copy link
Collaborator

rezrah commented Jul 26, 2024

Look much better, thanks @joshfarrant.

I think this regression occurred because we don't have dedicated stories for the validation states that are tracked in our VRT suite. If possible, could you please add some stories under FormControl enabling the validationStatus props for both success and error states. We'll then be able to see visual diffs in future.

@joshfarrant
Copy link
Contributor Author

joshfarrant commented Jul 26, 2024

Good suggestion, thanks @rezrah

That's added, although the changes are fairly lost in the sea vrt spec files updates.

I'll rebase and get rid of those changes once #670 is in

EDIT: Rebased ✅

@joshfarrant joshfarrant force-pushed the joshfarrant/formcontrol-validation-alignment-bug branch from 28c12c7 to 4577fce Compare July 26, 2024 13:05
@joshfarrant joshfarrant force-pushed the joshfarrant/formcontrol-validation-alignment-bug branch from 2364f04 to 4577fce Compare July 26, 2024 13:30
@joshfarrant joshfarrant merged commit 9cb14ed into main Jul 29, 2024
34 checks passed
@joshfarrant joshfarrant deleted the joshfarrant/formcontrol-validation-alignment-bug branch July 29, 2024 14:08
@primer-css primer-css mentioned this pull request Jul 29, 2024
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.

2 participants