-
Notifications
You must be signed in to change notification settings - Fork 119
Feat/form components changes #4311
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
base: main
Are you sure you want to change the base?
Conversation
|
🦋 Changeset detectedLatest commit: 0a09367 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
View your CI Pipeline Execution ↗ for commit 0a09367.
☁️ Nx Cloud last updated this comment at |
Size Change: +333 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0a09367:
|
b5c6a75
to
51277f5
Compare
Paste
|
Project |
Paste
|
Branch Review |
feat/form-components-changes
|
Run status |
|
Run duration | 06m 04s |
Commit |
|
Committer | PixeledCode |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
1
|
|
0
|
|
0
|
|
0
|
|
66
|
View all changes introduced in this branch ↗︎ |
Tests for review
cypress/integration/link-checker/index.spec.ts • 1 failed test • Paste Docsite integration tests
Test | Artifacts | |
---|---|---|
Broken link checker > recursively check all website links for any broken links |
Screenshots
|
color: "colorTextWeaker", | ||
backgroundColor: "colorBackgroundBody", | ||
boxShadow: "shadowBorderWeak", | ||
boxShadow: "shadowBorderWeaker", |
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.
Suggestion: All the loading styles for buttons inherit from baseLoadingStyles right? If the color
and boxShadow
are denied there could we clean the button variants up here by removing them? Seems unnecessary to define them a second time. The only time we'd need to override it is for Inverse which is currently being done.
</Box> | ||
|
||
<Box as="span" hidden={!selected}> | ||
<SelectedIcon decorative color={highlighted && selected ? "colorTextIcon" : "colorTextPrimary"} /> |
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.
issue: I don't see any selected icon in the combobox. Could you share the design plz
element={`${element}_OPTIONAL_TEXT`} | ||
> | ||
{" "} | ||
(optional) |
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.
issue (blocking): We try not to have hard coded strings in our code. If we need this I would suggest exposing a prop for it and defaulting it. Consumers will need to be able to set this value for other languages. We normally use i18n in there somewhere. For example i18nErrorLabel
in Alert
@@ -40,6 +40,7 @@ export const RequiredDot: React.FC<React.PropsWithChildren<RequiredDotProps>> = | |||
display="block" | |||
height="4px" | |||
width="4px" | |||
position="relative" |
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.
👏 ty!
"& [data-paste-element='ICON']": { | ||
color: "colorTextIcon", | ||
}, |
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.
praise: this is cool, I didn't know you could do that!
https://paste-docs-git-feat-form-components-changes-twilio.vercel.app/