-
Notifications
You must be signed in to change notification settings - Fork 4
feat: 143-text-input-element refactore code #329
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
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
|
question: was there a solution design or discussion for form controls that I've missed? Forms are complex, nuanced and have a high-cost of change for consumers, so I don't think we should be implementing individual form controls until we've decided how we expect forms to be implemented at a high-level first. |
hi @kurtdoherty the ticket discussion is here here, this component has dependent to icons, so i create the actual code to see the changes |
|
Okay, well I don't think we've investigated forms enough to really get started on any of the individual controls yet. Without understanding how we expect products to facilitate form validation, submission, type safety and so on, any work on individual controls (like a text input) will be making prop interface decisions in the dark. |
hi @kurtdoherty |
Not sure at this stage. React 19 introduces some new form-related features that we need to investigate and assess; they may reduce the need to rely on |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
hi @kurtdoherty , thanks or the details, |
|
Yes, we should mark this as "on hold" for now. I'm planning to get some high-level decisions made to unblock this work before I go on leave at the end of next week. Those decisions should allow you to at least progress some of the text input work you've proposed here. |
|
FYI, we use react hook form and validation at the component level to avoid yup and display errors in the error label component with an error summary view at the top. Could just update the visuals and leave react 19 implications to v6 |
src/components/text-input/styles.ts
Outdated
| 'data-size'?: ElInputSizesEnum | ||
| } | ||
|
|
||
| export const ElInputErrorText = styled.p<ElInputErrorTextProps>` |
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.
assume this will move to a shared error text component for wider use, also getIconSize can be shared
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.
hi @adrian-reapit & @ksolanki7 thanks for the feedback,
question 1 : should we create new ticket for error text component or we can use the same ticket and split it by PR?
question 2: about the error text component since its used only for input should we create sub directory ? for example:
Component/
├── inputs/
│ ├── text-input/
│ ├── date-input/
│ ├── file-input/
│ ├── error-text/
│ ├── input-utils/
│ │ ├── get-icon-size.ts
or just keep it in different folder like this?:
Component/
├── another-component/
├── text-input/
├── date-input/
├── file-input/
├── error-text/
├── input-utils/
question 3:

since the render structure is like this, is it good enough? or you guys have any feedback ?
Notes: i will create new POC or the text-input, i have new idea about the prefix and suffix, i'll update it after this
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.
sorry for delay. there is a big plan missing for forms but we should be able to make some of the low level components. i have asked about it on figma
ErrorText Component Design Documenthi @ksolanki7 & @adrian-reapit OverviewThe Component APIProps
Key Features
Code ConceptTo see all example implementations, please check this POC. Usage ExamplesBasic Error Message<ErrorText isError={true}>This is an error message.</ErrorText> |
… remove icon dependent
|
hi @kurtdoherty & @adrian-reapit i just move the POC link to new sandbox since idk its wont update from my github repository please get access first to workspace : link to get request access here please let me know if the code still doesnt work |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
| } | ||
|
|
||
| export const TextInputWithPrefixAndSuffix: StoryObj<typeof TextInput> = { | ||
| name: 'TextInput with Required no Title', |
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.

Pull request checklist
Detail as per issue below (required):
as figma refenrece here Figma
schreenshoot
