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

feat(ui): #1773, #1774: ValueInput and SwapInput #1775

Merged
merged 24 commits into from
Sep 12, 2024
Merged

Conversation

VanishMax
Copy link
Contributor

@VanishMax VanishMax commented Sep 9, 2024

@VanishMax VanishMax changed the base branch from main to 1714-finish-the-assetselector-ui-component September 9, 2024 12:08
Copy link
Contributor

github-actions bot commented Sep 9, 2024

Visit the preview URL for this PR (updated for commit 5173bcb):

https://penumbra-ui-preview--pr1775-feat-1773-value-inp-ma6p3rle.web.app

(expires Thu, 19 Sep 2024 10:21:51 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1

@VanishMax VanishMax requested a review from a team September 9, 2024 12:12
@VanishMax VanishMax self-assigned this Sep 9, 2024
@VanishMax VanishMax marked this pull request as ready for review September 10, 2024 06:41
Copy link
Collaborator

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

LGTM

@TalDerei TalDerei self-requested a review September 11, 2024 19:31
Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

LGTM.

just to clarify, the white input range slider won't be displayed in production, right?

@VanishMax
Copy link
Contributor Author

@TalDerei Do you mean the arrows up-down in the number input? We can hide them but I don't see the necessity – it generally improves the a11y

@TalDerei
Copy link
Contributor

@TalDerei Do you mean the arrows up-down in the number input? We can hide them but I don't see the necessity – it generally improves the a11y

It's a minor nit, but I personally feel like that component feels outdated and actually not that particularly useful when I was interacting with the Storybook. I'm in favor of removing it.

@VanishMax
Copy link
Contributor Author

@TalDerei Hid the arrows

Won't merge this PR before #1771

image

Base automatically changed from 1714-finish-the-assetselector-ui-component to main September 12, 2024 10:16
# Conflicts:
#	packages/ui/src/AssetSelector/index.tsx
@VanishMax VanishMax merged commit de9bd06 into main Sep 12, 2024
8 checks passed
@VanishMax VanishMax deleted the feat/#1773-value-input branch September 12, 2024 11:41
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.

TextInput v2.1 update Implement <SwapInput/> UI component Implement <ValueInput/> UI component
4 participants