Skip to content

Conversation

@frankieyan
Copy link
Member

@frankieyan frankieyan commented Jul 6, 2023

Short description

This separates the CSS variables used for dividers and graphical elements' borders, from those used for input fields' borders.

To document these variables in Storybook, the four field-based components have had their documentation converted to MDX:

image

By having this separation, we'd be able to take advantage of the new inputs colours from Design (ref).

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

Minor. It's a bit of a grey area as the new variables are instantiated with the old --reactist-divider colours, keeping backwards compatibility, and existing overrides using these variables should not break.

@frankieyan frankieyan requested review from a team and Bloomca and removed request for a team July 6, 2023 02:09
Copy link
Contributor

@Bloomca Bloomca left a comment

Choose a reason for hiding this comment

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

Change looks good. I am not sure what I need (if I need to do anything) on Chromatic, though.

@frankieyan
Copy link
Member Author

@Bloomca I am wondering if I should stick with the old story names (i.e. ending in "Story") so that Chromatic can match up the diffs, or leave it as is and accept the new diffs. Thoughts? cc. @gnapse

Before After
image image

@gnapse
Copy link
Contributor

gnapse commented Jul 7, 2023

I am wondering if I should stick with the old story names (i.e. ending in "Story") so that Chromatic can match up the diffs, or leave it as is and accept the new diffs. Thoughts?

We should not keep a name only because of Chromatic. If we really find the name change to be what we want, we should do the change, even if inconvenient.

I do agree the name change is for the better.

@frankieyan frankieyan force-pushed the frankie/field-border-colour branch from 1460a43 to cf995a6 Compare July 7, 2023 19:33
@frankieyan frankieyan merged commit 1feff1c into main Jul 10, 2023
@frankieyan frankieyan deleted the frankie/field-border-colour branch July 10, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants