Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

likhith/Text field UI component #234

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

likhith-deriv
Copy link

@likhith-deriv likhith-deriv commented Jul 6, 2022

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Addresses 61199#

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Package update

Author Checklist:

Please check relevant items before requesting for review.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Jul 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
deriv-components ✅ Ready (Inspect) Visit Preview Jul 14, 2022 at 11:42AM (UTC)

@likhith-deriv likhith-deriv changed the title [WIP] likhith/Text field UI component likhith/Text field UI component Jul 10, 2022
@likhith-deriv likhith-deriv marked this pull request as ready for review July 10, 2022 06:10
Copy link
Contributor

@yashim-deriv yashim-deriv left a comment

Choose a reason for hiding this comment

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

Some comments, I see that you combine TextArea and Input within one wrapper component. I think we should split it if it has specific behaviour. For the props, do use a clearer name like success should be using a more detailed name like either success_message. Another good way is to structure it in another way instead of a flat props. Example

hint_text props:

hint_text: {
    error: "",
    success: "",
    hint:""
}

src/components/core/text-field/text-field.tsx Outdated Show resolved Hide resolved
src/components/core/text-field/text-field.tsx Outdated Show resolved Hide resolved
src/components/core/text-field/text-field.tsx Show resolved Hide resolved
yashim-deriv
yashim-deriv previously approved these changes Jul 13, 2022
@@ -0,0 +1,450 @@
import zxcvbn from 'zxcvbn';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should lazy load zxcvbn library since this is a huge package. We only use Password Meter at a few places. So, I think it's better to lazy load.

Copy link
Author

Choose a reason for hiding this comment

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

Will incorporate this suggestion

@balakrishna-deriv
Copy link
Contributor

balakrishna-deriv commented Jul 14, 2022

There is a lot of removal from package-lock.json. Can you please check it again?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants