-
Notifications
You must be signed in to change notification settings - Fork 63
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
Refactor FormField #2329
Refactor FormField #2329
Conversation
🦋 Changeset detectedLatest commit: 7cfef1c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
Size Change: -424 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
packages/form-field/src/FormFieldInputContainer/FormFieldInputContainer.styles.ts
Outdated
Show resolved
Hide resolved
&::placeholder { | ||
color: ${palette.gray.dark1}; | ||
font-weight: ${fontWeights.regular}; | ||
color: ${isDarkMode | ||
? color.dark.text.primary.default | ||
: palette.gray.base}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the placeholder tokens should be left as-is. They match the tokens used for disabled but shouldn't use the disabled tokens
I don't see placeholder samples in the latest figma file. @alvenw can we add variants to the inputs that provide specs for the placeholder colors?
packages/form-field/src/FormFieldInputContainer/FormFieldInputContainer.styles.ts
Outdated
Show resolved
Hide resolved
font-size: inherit; | ||
line-height: inherit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there still a need to include these inherit
values if we set these attributes further down?
@@ -0,0 +1,200 @@ | |||
export const countries = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might name this file constants.ts
and capitalcase COUNTRIES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its just for our shared stories - i can, but we could also easily rm and use another example
[Variant.Placeholder]: { | ||
[State.Default]: gray.dark1, | ||
[State.Hover]: gray.dark1, | ||
[State.Focus]: gray.dark1, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would sort this the same way it is done for lightModeTextColors
* run chromatic * add something back * reset * fix placeholder color * disbaled tokens * fix? * low contrast * light refactor * another fix, another story * last commit pre-separating tokens * card * run fix * validate * add back disabled and placeholder * caught bug * Disable chromatic * placeholder token changeset * confused
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changesFor new components
🧪 How to test changes