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

[Bug]: NumberField without Increment and Decrement #613

Closed
2 tasks
hsellik opened this issue Jun 16, 2024 · 8 comments · Fixed by #623
Closed
2 tasks

[Bug]: NumberField without Increment and Decrement #613

hsellik opened this issue Jun 16, 2024 · 8 comments · Fixed by #623
Labels
bug Something isn't working

Comments

@hsellik
Copy link
Contributor

hsellik commented Jun 16, 2024

Reproduction

https://codesandbox.io/p/devbox/shadcn-vue-numberfield-padding-xnwmrm

Describe the bug

When removing NumberFieldDecrement and NumberFieldIncrement around NumberFieldInput, the input has large padding on both sides.

image

My temporary fix was to add px-1 class to NumberFieldInput like so:
<NumberFieldInput class="px-1" />

The NumberFieldInput component must have a class prop to override the padding:

<script setup lang="ts">
import { NumberFieldInput } from 'radix-vue';
import { cn } from '@/lib/utils';
import type { HTMLAttributes } from 'vue';

const props = defineProps<{
  class?: HTMLAttributes['class'];
}>();
</script>

<template>
  <NumberFieldInput
    :class="
      cn(
        'flex h-10 w-full rounded-md border border-input bg-background px-8 py-2 text-sm text-center ring-offset-background placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50',
        props.class,
      )
    "
  />
</template>

System Info

System:
    OS: macOS 14.5
    CPU: (8) arm64 Apple M1
    Memory: 91.56 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    Yarn: 1.22.21 - ~/.nvm/versions/node/v20.10.0/bin/yarn
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
    pnpm: 8.15.5 - ~/.nvm/versions/node/v20.10.0/bin/pnpm
  Browsers:
    Chrome: 125.0.6422.142
    Safari: 17.5
  npmPackages:
    @vueuse/core: ^10.11.0 => 10.11.0 
    radix-vue: ^1.8.3 => 1.8.3 
    vue: ^3.4.29 => 3.4.29

Contributes

  • I am willing to submit a PR to fix this issue
  • I am willing to submit a PR with failing tests
@hsellik hsellik added the bug Something isn't working label Jun 16, 2024
@Saeid-Za
Copy link
Contributor

Saeid-Za commented Jun 16, 2024

Hello There !
To remove the extra padding, go to NumberFieldInput.vue component in your project, then replace the px-10 py-2 class to p-2.

@hsellik
Copy link
Contributor Author

hsellik commented Jun 16, 2024

Adding p-2 will break the NumberField when the Decrement and Increment buttons are used in another place in the project (which is the case for me). A prop could solve that problem, but the user experience would feel awkward.

image

There are also other temporary options such as using flex justify-between instead of relative in NumberFieldContent and absolute in Increment and Decrement, but that would mean that the + and - buttons are not in the input anymore. Though they would flexibly work for both situations.

@Saeid-Za
Copy link
Contributor

Oh, I understand, how about adding a boolean prop to the input, to conditionally change the padding based on the presense of the Inc/Dec buttons?

@hsellik
Copy link
Contributor Author

hsellik commented Jun 17, 2024

As stated above, a boolean prop could solve the problem, but developer experience would be lacking. It would not be much different from the temporary solution offered in the bug description.

@sadeghbarati
Copy link
Collaborator

I think we could use CSS feature like has to solve this issue

@hsellik
Copy link
Contributor Author

hsellik commented Jun 18, 2024

I tried looking into has, but unfortunately, couldn't come up with a good solution. It can also be a problem when it comes to browser support?

@Saeid-Za , @sadeghbarati , what do you think about a solution similar to the one offered by radix-vue?

flex would be used on NumberFieldContent. This would also mean that the "input CSS" (focus border, etc.) on NumberFieldInput would be moved to NumberFieldContent. I can make a PR tomorrow so you can see what I mean.

This would eliminate complex peer checking, but allow to use the component in different configurations:

  • with decrement/increment
  • without decrement/increment
  • with only decrement
  • with only increment.

hsellik added a commit to hsellik/shadcn-vue that referenced this issue Jun 19, 2024
Change the NumberField, so it could be used
in different configuration without extra
effort from the developer.
hsellik added a commit to hsellik/shadcn-vue that referenced this issue Jun 19, 2024
@icarus12345
Copy link

icarus12345 commented Jun 22, 2024

That are not issue of shadcn, just add class first:px-1 last:pr-1 to the input to resolve that

@hsellik
Copy link
Contributor Author

hsellik commented Jun 22, 2024

While I agree, it's easy to fix, it's also a question of developer experience. Using a Number Field without decrement and increment is quite a common use case. Should developers modify the component themselves for this obvious use case, or should shadcn-vue provide a working solution out-of-the-box?

At the very least, there should be a class prop for the NumberFieldInput component and an example in the documentation so developers don't have to think about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants