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

feat(date-textbox): added floating label support #2157

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Conversation

agliga
Copy link
Contributor

@agliga agliga commented Apr 15, 2024

Description

  • Added floating label support for date-textbox (followed similar pattern to placeholder text)
  • Added tests
  • Note After adding server tests, navigator.language was breaking on the server. Added a guard against that.
  • There is an open discussion about keeping the floating label always floated like we do with phone input. Regardless, if that is the way we are going to go, we will need a follow up after phone input is merged to add support for that.

References

#2035

Screenshots

Screenshot 2024-04-15 at 9 23 12 AM Screenshot 2024-04-15 at 9 23 37 AM Screenshot 2024-04-15 at 9 23 40 AM

@agliga agliga self-assigned this Apr 15, 2024
Copy link

changeset-bot bot commented Apr 15, 2024

🦋 Changeset detected

Latest commit: bc4bbee

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

This PR includes changesets to release 1 package
Name Type
@ebay/ebayui-core 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

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.

Everything looks good, except there's some weirdness with the option in the docs. It says, "If set then shows this text as the floating label," but entering a string doesn't work:

image

The docs are clear about setting the range of dates floating labels, but using the directions provided is still unclear about how to set it for one date.

@agliga
Copy link
Contributor Author

agliga commented Apr 16, 2024

Everything looks good, except there's some weirdness with the option in the docs. It says, "If set then shows this text as the floating label," but entering a string doesn't work:

image The docs are clear about setting the range of dates floating labels, but using the directions provided is still unclear about how to set it for one date.

This is due to storybook not fully supporting text/array. If you put it in quotes, it works fine.
Screenshot 2024-04-16 at 8 45 43 AM

Im not sure what more we can say on this. If you have any suggestions, let me know or commit it.
This is using the same docs as inputPlaceholderText. It shows that it can be an array or text.

@agliga
Copy link
Contributor Author

agliga commented Apr 16, 2024

I added it, although I don't think its that big of a problem since this is only affecting people entering information in the storybook. In the code when they actually use it, all strings need to be in quotes anyways.

@ArtBlue
Copy link
Contributor

ArtBlue commented Apr 16, 2024

I added it, although I don't think its that big of a problem since this is only affecting people entering information in the storybook. In the code when they actually use it, all strings need to be in quotes anyways.

Yea, I realize it's just a storybooks docs thing and works well otherwise, but if the docs appear to break when a dev uses those docs in the most intuitive way they know (just enter text) with no other directions otherwise (need to wrap the text in quotes) to help them be able to use the docs, it's far less than ideal.

I mean, part of our job is to help improve the DX, and clarity is a big part of that. Is it a big deal? Maybe. Maybe not. But does the change improve the DX? I would say that's categorically a 'yes.' When you consider that the change doesn't impact code in any way and simply provides clarity, I would say it would be wrong NOT to do it.

@agliga agliga merged commit 4f8e826 into 13.4.0 Apr 16, 2024
2 checks passed
@github-actions github-actions bot mentioned this pull request Apr 16, 2024
@github-actions github-actions bot mentioned this pull request Apr 29, 2024
@agliga agliga deleted the floating-label-date branch April 30, 2024 15:04
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.

None yet

2 participants