feat: add selects to ui library #1290
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a new Select component system (app/components/Select/Base.vue and Select/Field.vue) and migrates many native elements across the app to SelectField usage (Org members, package controls, pagination, settings, list controls, etc.). Tooltip components (app/components/Tooltip/Base.vue and Tooltip/App.vue) gain a configurable to prop and teleport forwarding. Replaces several inline title-based tooltip spans with TooltipApp components. Updates Input/Base.vue classes for large size and adds appearance-none. Adds unit and accessibility tests for the new select components and tooltip teleport behaviour. Possibly related PRs #1268 — modifies tooltip usage and adds/adjusts tooltip teleport/target handling, overlapping the new to prop and Tooltip App/Base changes. #1173 — edits Input/Base.vue styling and class names, overlapping the Input/Base.vue visual changes in this PR. #801 — changes Tooltip/App.vue and Tooltip/Base.vue teleport/prop behaviour, directly related to the telemetry/prop forwarding introduced here. Suggested reviewers danielroe knowler 🚥 Pre-merge checks | ✅ 1 ✅ Passed checks (1 passed) Check name Status Explanation Description check ✅ Passed The pull request description clearly outlines the addition of two Select components (SelectBase and SelectField) to the UI library, explaining design decisions, naming conventions, sizing strategies, and implementation scope. ✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches [ ] 📝 Generate docstrings 🧪 Generate unit tests (beta) [ ] Create PR with unit tests [ ] Post copyable unit tests in a comment No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments app/components/Select/Field.vue (1) 52-59: Consider the v-bind placement order. The v-bind="selectAttrs" on line 58 is placed after :disabled, size, class, and v-model. In Vue 3, later bindings override earlier ones, so if selectAttrs contains keys like disabled or class, they will override the explicit bindings above. If this is intentional to allow consumer customisation, consider documenting it. If not, move v-bind="selectAttrs" before the explicit props you want to protect: ♻️ Suggested reordering (if overrides are not intended) <SelectBase + v-bind="selectAttrs" :disabled="disabled" size="none" class="appearance-none group-hover/select:border-fg-muted" :class="[SELECT_FIELD_SIZES[size], block ? 'w-full' : 'w-fit']" v-model="model" - v-bind="selectAttrs" :id="id" > Comment @coderabbitai help to get the list of available commands and usage tips. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/PaginationControls.vue (1)
154-176:⚠️ Potential issue | 🟡 MinorRemove the duplicate chevron icon.
The
SelectFieldcomponent already renders its own chevron icon internally (viaSelect/Field.vuelines 69-73). The chevrondivat lines 170-175 should be removed to avoid displaying two dropdown indicators.🔧 Proposed fix
<SelectField :label="$t('filters.pagination.items_per_page')" hidden-label id="page-size" v-model="pageSizeSelectValue" `@change`="handlePageSizeChange" :items=" PAGE_SIZE_OPTIONS.map(size => ({ label: size === 'all' ? $t('filters.pagination.all_yolo') : $t('filters.pagination.per_page', { count: size }), value: String(size), })) " /> - <div - class="flex items-center absolute inset-ie-2 top-1/2 -translate-y-1/2 text-fg-subtle pointer-events-none" - aria-hidden="true" - > - <span class="i-carbon-chevron-down w-3 h-3" /> - </div> </div>
🧹 Nitpick comments (2)
app/components/Select/Base.vue (1)
21-32: Consider removing inlinefocus-visiblestyling in favour of the global rule.The inline
focus-visible:outline-accent/70class may conflict with or override the global focus-visible styling defined inmain.cssfor select elements. Based on learnings, focus-visible styling for selects should rely on the global rule:select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }.If the global rule provides sufficient styling, consider removing the inline focus-visible classes to maintain consistency and avoid specificity conflicts.
♻️ Proposed simplification
<select v-model="model" - class="bg-bg border border-border font-mono text-fg placeholder:text-fg-subtle transition-[border-color,outline-color] duration-300 hover:border-fg-subtle outline-2 outline-transparent outline-offset-2 focus:border-accent focus-visible:outline-accent/70 disabled:(opacity-50 cursor-not-allowed)" + class="bg-bg border border-border font-mono text-fg placeholder:text-fg-subtle transition-[border-color,outline-color] duration-300 hover:border-fg-subtle disabled:(opacity-50 cursor-not-allowed)" :class="[SELECT_SIZES[size]]"app/components/Select/Field.vue (1)
22-33: Consider refining the type definitions forinputAttrsandlabelAttrs.The type
HTMLSelectElement['attributes']returnsNamedNodeMap, which isn't ideal for declaring passthrough attributes. A more precise type would be a record-style type or using Vue's native attribute types.♻️ Suggested type refinement
export interface SelectFieldProps extends SelectBaseProps { items: { label: string; value: string; disabled?: boolean }[] size?: keyof typeof SELECT_FIELD_SIZES - inputAttrs?: HTMLSelectElement['attributes'] & Omit<SelectBaseProps, 'size' | 'id'> + inputAttrs?: Record<string, unknown> & Omit<SelectBaseProps, 'size' | 'id'> label?: string - labelAttrs?: HTMLSelectElement['attributes'] & Omit<SelectBaseProps, 'size' | 'id'> + labelAttrs?: Record<string, unknown> /** Visually hide label */ hiddenLabel?: boolean id: string /** Render select full width */ block?: boolean }
|
Well, all the suggestions work too, so I accepted them 🤞 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/Org/MembersPanel.vue (1)
466-480: Drop the extra:valuebinding onSelectField.
modelValuealready controls the selection, so:valueis redundant and can introduce a second source of truth.Proposed fix
<SelectField :label="$t('org.members.change_role_for', { name: member.name })" hidden-label :id="`role-${member.name}`" :model-value="member.role" :name="`role-${member.name}`" block size="sm" :items="[ { label: getRoleLabel('developer'), value: 'developer' }, { label: getRoleLabel('admin'), value: 'admin' }, { label: getRoleLabel('owner'), value: 'owner' }, ]" - :value="member.role" `@update`:modelValue="value => handleChangeRole(member.name, value as MemberRole)" />
There was a problem hiding this comment.
I like the use of the monospace font.
Eventually, we’ll need to more broadly reckon with the 3:1 non-text contrast requirement (1.4.11 Non-text Contrast Level AA; further explanation), but for now I think this is fine (this PR doesn’t introduce the non-text contrast failure).
I didn’t do a full code review, so I defer to Vue-knowers for that.
3 participants
Continuing work on the UI core. Added Selects.
Сreated two selects:
SelectBaseas a basic, styled select without any unnecessary logic, andSelectFieldas a full-fledged field with all the associated logic.For the props, I based them on uno-tailwind patterns and the approaches adopted in
nuxt-ui. I think these will be more suitable names for DX in the long term.SelectBaseandSelectFieldhave different size pairs because in a default select - dropdown has an extra size and we cannot control it in all browsers.I continued to use the same size variations as the input to ensure they look consistent next to each other (there's a difference of ~0.2-0.5px in some sizes, I'll address that later)
Applied final selects to the entire site and checked - I hope I didn't miss anything. Fully covered with tests