-
Notifications
You must be signed in to change notification settings - Fork 19
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(tw): file input component #4644
base: master
Are you sure you want to change the base?
Conversation
feat: Boolean field input
The file input is integrated with the FieldInput component, but it will be refactored if/when the other form PR is merged. |
@@ -96,6 +99,7 @@ | |||
--text-color-button-outline-hover: var(--color-blue-700); | |||
--text-color-button-disabled: var(--color-gray-600); | |||
--text-color-button-disabled-hover: var(--color-gray-600); | |||
--text-color-button-filter: var(--color-blue-500); |
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.
When used in combination with --background-color-button-filter
, the color contrast is not strong enough. If set to --color-blue-800
, it satisfies contrast guidelines. This will affect the rest of the theme. Should I change this now? I would like to conduct an extensive review of the components and fix the colors. Should I save it for that PR?
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 have one comment regarding the theme. See comment below.
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.
Works! Some minor code clean up comments.
I also don't really like having the filterwell style file in there. It suggests to me that it's an multi-input. It also vexes me that when I mouse over it suggests that is is clickable, but I need to press the button to actually interact.
Should we also support drag and drop?
I tried looking for this input in the design figma, but couldn't find it. Do you know where to find it? nvm found it, was looking over it. It looks the same as the design 👍
</label> | ||
<InputFile | ||
id="file-input-demo" | ||
@update:model-value="(value) => (file = value)" |
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.
ts error
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.
What is the precise error that you are seeing? I'm not seeing any errors. I've restarted my local ts servers and I still do not see an error.
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.
implicit any type
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.
Looks great, 3 thing that should be improved imo;
1: clicking on the input ( next to the button ) does not trigger the file selector
2: the btn label gets the name of the field instead of labeling the action ( select , browse .. )
3 the text mentions the selection of multiple files , i was not able to select multiple files ( maybe this is a separate pr ? , in that case maybe change text for now )
|
Waiting for PR #4664 before continuing with this PR |
What are the main changes you did
Warning
the purpose of this PR is to add the structure and basic functionality. It will be merged with the other input PR and refactored.
How to test
Checklist