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(phone-input): new module #2259

Merged
merged 3 commits into from
Apr 10, 2024
Merged

feat(phone-input): new module #2259

merged 3 commits into from
Apr 10, 2024

Conversation

saiponnada
Copy link
Contributor

@saiponnada saiponnada commented Feb 1, 2024 •
edited

Fixes #1601

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

Created a new Phone-Input module. Made changes to button to address variants for phone-Input.
Added docs to Phone-Input and floating-label.

Notes

Screenshots

image image

Variants

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@saiponnada
Copy link
Contributor Author

I would like to get the initial feedback on html structure and few questions listed below,

  • Regarding BEM, I tried not to create any new classes except for menu-button__item-content and used regular menu-button and textbox control classes to override.
  • Should the aria-label be on the button instead of svg flag inside the button?
  • Should we have a prefix to the button saying dialing code or country code to better convey this to AT?
  • Below image is when resized to 319px. Should there be a max width for this control at this size? May be a question to Design?
    image

Base automatically changed from 17.2.0 to master February 28, 2024 16:12
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 4, 2024 •
edited

🦋 Changeset detected

Latest commit: 829a953

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

This PR includes changesets to release 1 package
Name Type
@ebay/skin Minor

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

@saiponnada saiponnada changed the base branch from master to 17.3.0 March 4, 2024 17:58
@saiponnada saiponnada marked this pull request as ready for review March 12, 2024 19:13
@saiponnada saiponnada self-assigned this Mar 12, 2024
agliga
agliga reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good.
There are some issues we need to fix before it goes out like with the shifting and some minor docs changes.

docs/_includes/phone-input.html Outdated Show resolved Hide resolved
docs/_includes/phone-input.html Outdated Show resolved Hide resolved
src/less/phone-input/phone-input.less Outdated Show resolved Hide resolved
agliga
agliga reviewed Mar 20, 2024
View reviewed changes
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

Couple more issues I found when doing ebayui integration

dist/phone-input/phone-input.css Outdated Show resolved Hide resolved
docs/_includes/floating-label.html Show resolved Hide resolved
Base automatically changed from 17.3.0 to master March 21, 2024 19:02
@agliga agliga changed the base branch from master to 17.4.0 March 22, 2024 16:27
ArtBlue
ArtBlue requested changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks nice so far. There are a few things to consider.

docs/src/js/main.js Show resolved Hide resolved
src/less/phone-input/phone-input.less Outdated Show resolved Hide resolved
src/less/phone-input/phone-input.less Outdated Show resolved Hide resolved
dist/tokens/evo-light.css Outdated Show resolved Hide resolved
src/less/button/button.less Outdated Show resolved Hide resolved
docs/src/js/main.js Show resolved Hide resolved
src/less/phone-input/phone-input.less Outdated Show resolved Hide resolved
src/less/phone-input/phone-input.less Show resolved Hide resolved
agliga
agliga reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

Overall lgtm, just a couple of docs and minor style concerns

docs/_includes/phone-input.html Outdated Show resolved Hide resolved
src/less/phone-input/phone-input.less Show resolved Hide resolved
agliga
agliga approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

LGTM
Overall good job!

ArtBlue
ArtBlue requested changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Nice work!!! Everything looks good except,

  1. The question about the prefix possibly being inside <input> if we're going to be using a mask in ebay ui.
  2. The way we're handling disabled flags.

src/less/flag/flag.less Outdated Show resolved Hide resolved
@saiponnada saiponnada requested a review from ArtBlue April 10, 2024 18:24
ArtBlue
ArtBlue approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Nice!!!

@saiponnada saiponnada merged commit fff8608 into 17.4.0 Apr 10, 2024
1 check passed
@agliga agliga deleted the 1601-phone-input branch April 10, 2024 21:28
@github-actions github-actions bot mentioned this pull request Apr 16, 2024
Merged
ianmcburnie
ianmcburnie reviewed Apr 17, 2024
View reviewed changes
docs/_includes/phone-input.html
<span class="btn__cell">
<span class="btn__text">
<svg class="flag flag--us" focusable="false" height="18" width="24" aria-hidden="true" data-country-code="1"
aria-label="Country: United States of America (+1)">
Copy link
Contributor

Choose a reason for hiding this comment

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

@saiponnada I notice this example code has the aria-label on the svg instead of the button.

ianmcburnie
ianmcburnie reviewed Apr 17, 2024
View reviewed changes
docs/_includes/floating-label.html
Number</label>
<span class="textbox">
<span id="floating-phone-prefix-0">+1</span>
<input class="textbox__control" type="tel" aria-label="area code + phone number"
Copy link
Contributor

Choose a reason for hiding this comment

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

@saiponnada As we already have a label element doing the job, we can remove aria-label here.

@github-actions github-actions bot mentioned this pull request Apr 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianmcburnie ianmcburnie ianmcburnie left review comments

@ArtBlue ArtBlue ArtBlue approved these changes

@agliga agliga agliga approved these changes

@LuLaValva LuLaValva Awaiting requested review from LuLaValva

Assignees

@saiponnada saiponnada

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

phone-input: NEW module (listbox-button + textbox)
4 participants
@saiponnada @ianmcburnie @ArtBlue @agliga