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 Field add data-placeholder attribute and delete improv #808

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

max-got
Copy link

@max-got max-got commented Oct 24, 2024

Changes

Currently i am porting Origin UI to Svelte (See). As i was trying to port the Components over i saw that you are handling the placeholder styles with:

<component class="aria-[valuetext=Empty]:something"></component>

The React Aria Datepicker changes the aria-valuetextbased on the locale. So the styling would only work if the locale isen. If you decide to implement translation based on the locale in the future, the current implementation would break styling.

On top of that, as i was writing the tests for that i saw that a CTRL + A + BACKSPACE / DELETE deletion was not reliably possible. I don't really know if you want to change that. Again, the React Aria implementation doesn't have that. You can't even CTRL + A a Segment (which i find weird). 🤷‍♂️

1. TLDR: Improved placeholder handling

  • Added data-placeholder="true" attribute to empty date/time segments
  • Replaced the previous aria-[valuetext=Empty]:text-muted-foreground styling approach
  • This change makes the placeholder styling more robust and locale-independent
    • Previous implementation relied on valuetext which varies based on locale
    • Example: styling would only work if you decide to never implement i18n
    • New approach works consistently, even if changes are happening in the future

2. TLDR: Enhanced deletion behavior

  • Added proper support for clearing date/time segments when fully selected
  • Now supports both:
    • CTRL + A + Backspace
    • CTRL + A + Delete

Test Coverage

Added two tests:

  1. Verification of clearing date segments when fully selected:
//date-field.test.ts
  it("should clear date segments when fully selected and backspace is pressed", async () => {
    // Tests CTRL + A + Backspace functionality across all segments
  });
  1. Verification of data-placeholder attribute behavior:
//date-field.test.ts
it("should apply data-placeholder attribute to empty segments", async () => {
// Tests initial state, filling, clearing, and partial input scenarios
});

Visual Changes

Empty date/time segments now use data-placeholder attribute for styling
Visual appearance remains the same, but implementation is more robust

Testing Instructions

Open the DateField component (or based on that)
Try selecting all text in any segment (CTRL + A)
Verify deletion works with both:

  1. Backspace key
  2. Delete key

Verify placeholder styling appears correctly when segments are empty

Copy link

changeset-bot bot commented Oct 24, 2024

⚠️ No Changeset found

Latest commit: 2d8172a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Contributor

github-actions bot commented Oct 25, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
bits-ui ✅ Ready (View Log) Visit Preview 2d8172a

@huntabyte
Copy link
Owner

Wow, thanks for this! I will give it a look very soon!

@huntabyte
Copy link
Owner

huntabyte commented Oct 26, 2024

CleanShot.2024-10-26.at.13.52.27.mp4

Here's some interesting behavior that we should perhaps account for. Should we deselect the text once it is complete? If it's fully selected like that, it feels like it would just keep overwriting everything selected.

@max-got
Copy link
Author

max-got commented Oct 26, 2024

Interesting, is something like the that desirable?

CleanShot.2024-10-26.at.21.27.37-converted.mp4

@huntabyte
Copy link
Owner

If the whole block is highlighted, my instinct says the next number I type should replace the entirety of the highlighted block, similar to how it would work if you highlighted editable text and started typing, right?

@max-got
Copy link
Author

max-got commented Oct 26, 2024

I am totally on your side, but with your current implementation you aren't able to input a single value / replace the entirety, because you prepend the value with zeros. Therefor (bc of consistency) my implementation is "more" correct i think.

In the Svelte 4 Version of bits or in React Aria that isn't the case. Here you can input "5" in the year segment for example and the value would stay that way (which i prefer) instead of 0005. If you want to change that behavior your idea would be more correct.

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.

2 participants