Conversation
📝 WalkthroughWalkthroughAdds a new InputBase Vue component (props: disabled, size, noCorrect; emits: focus, blur; exposes focus() and blur()). Replaces numerous native input elements across the app with InputBase (header, search, auth/connector modals, compare UI, org panels, package controls, date inputs, OTP input), standardising classes and sizes. Adjusts AppHeader centre alignment to use Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Filter/Panel.vue (1)
116-119:⚠️ Potential issue | 🟡 MinorSwitch from
@inputand:valueto@update:modelValueand:model-valuewith InputBase.InputBase defines only
update:modelValue(plusfocusandblurevents), not native input events. Using@inputand:valuewill not work correctly with this component. Update the handler signature to accept a string value directly instead of an Event object.Suggested fix
-function handleTextInput(event: Event) { - const target = event.target as HTMLInputElement - emit('update:text', target.value) -} +function handleTextInput(value: string) { + emit('update:text', value) +}<InputBase id="filter-search" type="text" - :value="filters.text" + :model-value="filters.text" :placeholder="searchPlaceholder" autocomplete="off" class="w-full min-w-25" size="medium" noCorrect - `@input`="handleTextInput" + `@update`:modelValue="handleTextInput" />
🧹 Nitpick comments (1)
app/components/Input/Base.vue (1)
2-8: Prop name shadows imported utility.The prop
noCorrect(line 8) shadows the importednoCorrectutility (line 2). While this works because you access the utility viaprops.noCorrect ? noCorrect : undefined, the naming could be clearer.Consider renaming the prop to
disableCorrector similar to avoid confusion with the imported utility object.♻️ Suggested rename
-import { noCorrect } from '~/utils/input' +import { noCorrect as noCorrectAttrs } from '~/utils/input' const props = withDefaults( defineProps<{ modelValue?: string size?: 'small' | 'medium' | 'large' noCorrect?: boolean }>(),Then update line 41:
- v-bind="props.noCorrect ? noCorrect : undefined" + v-bind="props.noCorrect ? noCorrectAttrs : undefined"
|
I'm afraid I need |
cc: @userquin |
…/npmx.dev into feat/add-input-component
|
Oh, it was wrong before as well. I've restored this fragment, I hope I'll find time later to polish these pages too and improve them for rtl |
we can add a custom rule/shorcut for this |
|
I think it would be useful to leave the option to skip the error for now (for example, by writing the reason and continuing to receive warnings) Essentially, these are places where we haven't yet worked out the RTL perfectly and we have a group of elements going from left to right. We probably need to collect all these elements and understand how they should look, and then continue following this guide for these situations... |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Use |
|
Huge thanks @userquin |
|
(it's merged) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/SearchBox.vue (1)
14-14:⚠️ Potential issue | 🟡 MinorRemove unused emits or wire them up.
The
blurandfocusemits are defined but never called—the handlers at lines 117–118 only update local state. If parent components rely on these events, addemit('focus')andemit('blur')to the handlers; otherwise, remove the unuseddefineEmitsdeclaration.Option A: Remove if unused
-const emit = defineEmits(['blur', 'focus'])Option B: Wire up if needed
- `@focus`="isSearchFocused = true" - `@blur`="isSearchFocused = false" + `@focus`="isSearchFocused = true; emit('focus', $event)" + `@blur`="isSearchFocused = false; emit('blur', $event)"
🧹 Nitpick comments (1)
app/components/Input/Base.vue (1)
38-45: Consider renaming to avoid shadowing confusion.The prop
noCorrectshadows the importednoCorrectutility, making line 42 harder to read at a glance. Whileprops.noCorrectvsnoCorrectis technically unambiguous, this naming collision may confuse future maintainers.Consider renaming the prop (e.g.,
disableAutoCorrect) or aliasing the import (e.g.,import { noCorrect as noCorrectAttrs }).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
app/components/Input/Base.vue
Outdated
| ) | ||
|
|
||
| const emit = defineEmits<{ | ||
| 'update:modelValue': [value: string] |
There was a problem hiding this comment.
let' use defineModel for every prop/emit pair...
There was a problem hiding this comment.
Thanks, looks much better now (I don’t remember why I made it so complicated)
app/components/Input/Base.vue
Outdated
| defineExpose({ | ||
| focus: () => el.value?.focus(), | ||
| blur: () => el.value?.blur(), | ||
| getBoundingClientRect: () => el.value?.getBoundingClientRect(), |
There was a problem hiding this comment.
No, and it doesn't seem to be useful, I'll remove it, thanks
app/components/Org/MembersPanel.vue
Outdated
| :placeholder="$t('org.members.username_placeholder')" | ||
| v-bind="noCorrect" | ||
| class="w-full px-2 py-1.5 font-mono text-sm bg-bg border border-border rounded text-fg placeholder:text-fg-subtle transition-colors duration-200 focus:border-border-hover focus-visible:outline-accent/70" | ||
| noCorrect |
There was a problem hiding this comment.
normally we'd do kebab-cased no-correct in a template.
There was a problem hiding this comment.
Thanks, I forget every time, even if there is nothing more logical than kebab in the markup...
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
great work! just added a couple of small comments from a vue pov
4 participants
Added a common component for text-like inputs and used it throughout the site. The current search-field is used as a basis, with soft rounding, a light border, a bright border on hover, an accent border on focus, and an accent outline 2px from the input on focus
I went through all the places and made additional adjustments where needed. Where I could, I increased the size a bit (to 32px instead of 28.6). Also, if it was a group with a button or block, I adjusted the size for them as well.
I made the inputs a consistent height to avoid having to play exact matches in the future. Generally, leading-none with padding covered this issue, but sometimes it would break, so I explicitly specified the height as well.
(logic is covered for 100%, so I hope I configured everything correctly)
Closes #1172