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

fix(PasswordInput): Update placement of Show/Hide button on Large input size #1136

Merged
merged 8 commits into from
Oct 5, 2023

Conversation

silvalaura
Copy link
Collaborator

@silvalaura silvalaura commented Sep 20, 2023

Issue: #1088

What I did

Fix placement of show / hide button in large password input. Also added input size to storybook and to the list of the component's props.

Screenshots:

Before:
image

After:
image

Checklist

  • changeset has been added
  • Pull request description is descriptive
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • [n/a] I have added tests that prove my fix is effective or that my feature works

How to test

Test PasswordInput component in both the small and large size

@silvalaura silvalaura self-assigned this Sep 20, 2023
@silvalaura silvalaura requested a review from a team as a code owner September 20, 2023 19:25
@changeset-bot
Copy link

changeset-bot bot commented Sep 20, 2023

🦋 Changeset detected

Latest commit: 08ef260

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@silvalaura silvalaura linked an issue Sep 20, 2023 that may be closed by this pull request
@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

Copy link
Contributor

@orion-cengage orion-cengage left a comment

Choose a reason for hiding this comment

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

The original issue is fixed, but there needs to be some tweaks to polish it up.

  • The size seems inconsistent with other buttons on large inputs. The perfect example of sizing, spacing, etc is the Clear button on large text inputs.

  • image

  • It's 40px tall, equal distances from top, bottom and right side, and the focus ring doesn't run into the border of the input.

  • One of the problems specific to the password input, is that the show/hide button changes width depending on which state it is in. We need to pick a fixed width that accommodates both text labels so it doesn't change.

  • image

  • image

  • It appears when you change from medium to large input, all of the text in the input changes appropriately in size, but we're not changing the button. We need to either change it to the medium button variant, or just change the text style within the button.

  • image

  • image

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

@orion-cengage
Copy link
Contributor

Almost there. Just a couple little tweaks.

  1. Can we change the width/max-width of the button from 56px to 64px?
  2. Change width calculation on input element to "calc(100% - 64px)"
  3. The space to the right of the button isn't equal to the space above and below it. I think if we change the transform applied to the span wrapped around the button from (-60px, 8px) to (-72px, 8px) we'll be good.

In the end, we should end up with this:
image

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

@silvalaura silvalaura merged commit 1c446c2 into dev Oct 5, 2023
2 checks passed
@silvalaura silvalaura deleted the fix/passwordInputLarge branch October 5, 2023 15:36
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.

PasswordInput: Show button in size large is off center
2 participants