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.

Sign up for GitHub

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

Jump to bottom

feat(vwc-textfield): support multiple interactive icon buttons #837

Merged
merged 55 commits into from
Jun 3, 2021
Merged

feat(vwc-textfield): support multiple interactive icon buttons #837

merged 55 commits into from
Jun 3, 2021

Conversation

yinonov
Copy link
Contributor

@yinonov yinonov commented May 13, 2021 •
edited
Loading

  • disabled delegation
  • trailing icon affect
  • request enforce disable?
  • tests

image

@yinonov yinonov marked this pull request as draft May 13, 2021 20:13
@github-actions
Copy link

🚀

Latest successful build of the PR deployed here.

🚀

@yinonov yinonov added the blocked-by-issue Depending on other PRs to be commited label May 19, 2021
@yinonov
Copy link
Contributor Author

yinonov commented May 19, 2021

@jshenkman question regarding disabled textfield -
this PR enforces textfield disabled state on child interactive buttons.
there could be a case where consumer would need the ability to control icon button disabled state regardless of textfield itself

e.g. send icon button is disabled if no value

image
our 3 options:

  1. enforce disabled state
  2. not enforce disabled state
  3. provide a property to toggle enforcement

I believe this applies to @YonatanKra toggle button group as well #843

@jshenkman
Copy link
Contributor

Took a look at the PR and noticed a few things -

  1. In pill shape there is still extra padding for the label (for rounded it's fine, 16 px padding)

Screen Shot 2021-05-23 at 10 26 19

  1. Icon button size for default - the icon button should be 40px, it is currently 38px. padding is 7? should be 4
  2. Icon button size and Icon sizes for dense - the icon button should be 32px, it is currently 30px. And the icon should be 16 px it's currently 20

@yinonov
Copy link
Contributor Author

yinonov commented May 23, 2021 •
edited
Loading

Took a look at the PR and noticed a few things -

  1. In pill shape there is still extra padding for the label (for rounded it's fine, 16 px padding)

@jshenkman what is the desired value for the left padding? less 4px?

@yinonov yinonov added the Work in Progress You're welcome to have a look around and drop your early comments label May 27, 2021
@yinonov yinonov marked this pull request as ready for review June 3, 2021 06:03
@yinonov yinonov added Consumers Awaiting This feature is awaited for by one or more of our consumers enhancement New feature or request ready-as-poc ready-to-merge Type: Feature and removed Work in Progress You're welcome to have a look around and drop your early comments labels Jun 3, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 3, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@gullerya gullerya merged commit 464a725 into master Jun 3, 2021
@gullerya gullerya deleted the viv-555-textfield-multiple-interactive-icons branch June 3, 2021 19:59
@yinonov yinonov mentioned this pull request Jul 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gullerya gullerya gullerya approved these changes

Assignees
No one assigned
Labels
Consumers Awaiting This feature is awaited for by one or more of our consumers enhancement New feature or request ready-as-poc ready-to-merge Type: Feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yinonov @jshenkman @gullerya