feature: enhance FwbButton to support Vue Router navigation #417
Conversation
✅ Deploy Preview for sensational-seahorse-8635f8 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughFwbButton now supports Vue Router navigation via computed properties that choose between router-link, anchor, or button and dynamically bind the appropriate navigation attribute. The test suite is reorganized and expanded to cover router behavior, link/button states, variants, slots, loading, and custom classes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (5)
src/components/FwbButton/FwbButton.vue (2)
133-144:resolveComponent('router-link')try/catch likely never exercises the fallbackIn
buttonComponent:if (props.to) { try { return resolveComponent('router-link') } catch { console.warn('router-link component not found...') return 'a' } }In Vue 3,
resolveComponenttypically warns and returns a fallback rather than throwing on unresolved names, so thiscatchblock is unlikely to ever run. That means:
- In an app without
vue-router, you’ll probably still get a<router-link>tag instead of the intended<a>fallback.- The warning you emit is redundant with Vue’s own unresolved‑component warning but the behavior is unchanged.
If you want a real, tested fallback:
- Either rely on the string
'router-link'directly (and document thattorequires vue-router), or- Explicitly check the app context for a registered
router-linkcomponent and fall back to'a'when it’s missing, and add a test that mounts without a router-link stub and asserts an<a>is rendered.
133-149: Clarify interaction betweentagandto(especially fornuxt-link)When
props.tois set,buttonComponentalways resolves'router-link', ignoring thetagprop, whilelinkAttrstill special‑casesprops.tag === 'router-link' || props.tag === 'nuxt-link'.This leads to slightly surprising behavior:
tag="nuxt-link",to="/foo"→ component still resolvesrouter-link, notnuxt-link.tag="router-link",href="/foo", noto→ you correctly render a router-link withto="/foo"vialinkAttr/linkValue.If supporting Nuxt’s
NuxtLinkexplicitly is a goal, you may want to:
- Respect
tag === 'nuxt-link'whentois provided (e.g. resolve that instead of hard‑coding'router-link'), or- Document that
toalways uses vue-router’srouter-linkand that Nuxt users should rely on the RouterLink alias, notNuxtLink, when using this prop.Right now the behavior is workable but a bit implicit; tightening this contract would reduce surprises for Nuxt users.
src/components/FwbButton/tests/Button.spec.ts (3)
6-12: Strengthen router fallback coverage and expectationsThe Router Navigation tests nicely cover:
- Rendering as
router-linkwhentois provided (string and object).- Prioritizing
tooverhref.However, the behavior you comment about here:
// In a real environment without vue-router, this would fallback to 'a'is never actually tested—every router test provides a
'router-link'stub.Given the
buttonComponentlogic aims to fall back to<a>whenrouter-linkis unavailable, consider adding a dedicated test that:
- Mounts
FwbButtonwith atoprop but without stubbingrouter-link.- Asserts that the root element is an
<a>with the correcthref/toresolution (once the component logic is adjusted to actually fall back).This will protect the intended non‑router environment behavior from regressions and verify the try/catch (or alternative detection) actually works.
Also applies to: 56-122
161-184: Disabled link test codifies Vue’s internal behavior rather than the desired contractThis test:
// For links, disabled should not be an attribute, but the component might still render it as 'false' const disabledAttr = wrapper.find('a').attributes('disabled') expect(disabledAttr).toBe('false')asserts that
<a>carriesdisabled="false", which is really an artifact of how Vue binds attrs on non‑boolean‑aware elements, not a desirable API surface.If the intended contract is “links are never disabled via a
disabledattribute”, it would be clearer to assert that the attribute is absent instead:
- Either
expect(disabledAttr).toBeUndefined(), or- Update the component to avoid binding
disabledfor non‑button elements and then adjust this test.Right now the comment and assertion disagree on what “correct” looks like; aligning the expectation with the intended behavior will make future refactors to the component less brittle.
186-220: Loading state tests rely on brittle positional selectorsFor the loading tests you use:
const firstChild = wrapper.find('div:first-child') … const lastChild = wrapper.find('div:last-child')This depends heavily on the exact DOM structure (which
divcomes first/last under the root), so minor template changes (e.g., wrapping content, adding an extra container) will break these tests without changing behavior.You can make them more robust by targeting the known containers instead, e.g.:
- For prefix:
wrapper.find('div.mr-2')- For suffix:
wrapper.find('div.ml-2')or by scoping the search under the relevant wrapper element.
That way the tests assert semantic placement (spinner in the prefix/suffix container) rather than raw child indices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/FwbButton/FwbButton.vue(3 hunks)src/components/FwbButton/tests/Button.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-11T13:26:22.855Z
Learnt from: Sqrcz
Repo: themesberg/flowbite-vue PR: 338
File: docs/components/dropdown.md:243-261
Timestamp: 2025-01-11T13:26:22.855Z
Learning: The `disabled` prop in the FwbDropdown component is a boolean prop that defaults to `false` and controls whether the dropdown trigger button is disabled.
Applied to files:
src/components/FwbButton/FwbButton.vue
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/FwbButton/tests/Button.spec.ts (2)
96-113: “router-link availability” test doesn’t hit the fallback path and duplicates the happy path
it('should handle router-link availability gracefully', ...)currently mounts with arouter-linkstub registered, so it executes the same code path asshould render as router-link when to prop is providedand never exercises thecatch/fallback branch inbuttonComponent.Either:
- Remove this test as redundant, or
- Rework it to simulate the “no vue-router installed” scenario (e.g., mount without a
router-linkstub and assert that an<a>is rendered with the expected navigation attribute, once the fallback logic is fixed).This will better validate the new fallback behavior rather than duplicating the primary happy-path test.
195-229: Loading spinner position tests are slightly brittle to markup changesThe loading tests use selectors like
div:first-child/div:last-childto infer prefix/suffix placement. This works with the current structure but could break if additional wrapper<div>elements are introduced.If you want more resilient tests, consider targeting the known wrapper around the spinner (e.g., by class like
.mr-2/.ml-2) or asserting relative order in the rendered text/DOM rather than relying on “first-child/last-child” selectors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/FwbButton/FwbButton.vue(3 hunks)src/components/FwbButton/tests/Button.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-11T13:26:22.708Z
Learnt from: Sqrcz
Repo: themesberg/flowbite-vue PR: 338
File: src/components/FwbDropdown/FwbDropdown.vue:9-9
Timestamp: 2025-01-11T13:26:22.708Z
Learning: In Vue components with disabled states, ensure click handlers respect the disabled prop by adding early returns (e.g., `if (props.disabled) return`). The disabled attribute alone on elements isn't sufficient as the click handler can still execute.
Applied to files:
src/components/FwbButton/FwbButton.vue
📚 Learning: 2025-01-11T13:26:22.855Z
Learnt from: Sqrcz
Repo: themesberg/flowbite-vue PR: 338
File: docs/components/dropdown.md:243-261
Timestamp: 2025-01-11T13:26:22.855Z
Learning: The `disabled` prop in the FwbDropdown component is a boolean prop that defaults to `false` and controls whether the dropdown trigger button is disabled.
Applied to files:
src/components/FwbButton/FwbButton.vue
🔇 Additional comments (6)
src/components/FwbButton/tests/Button.spec.ts (4)
6-12: RouterLink stubbing and navigation tests look solidUsing a simple
RouterLinkStubwith atoprop and slot-only template is a clean way to test theto/router-link resolution. The router navigation tests (stringto, route-objectto, andtovshrefprecedence) exercise the new computed logic well and should guard most regressions around routing behavior.Also applies to: 65-131
231-359: Slots and variant tests provide good coverage of styling behaviorThe slot tests (prefix/suffix with and without outline gradients) and variant tests (colors, sizes, pill/square, outline, gradient, shadow) give broad coverage of the visual branches driven by the new props. Assertions are intentionally loose (e.g., checking presence of some classes instead of exact lists), which should keep them stable while still catching regressions in the basic styling behavior.
361-385: Custom class handling is well-coveredThe tests for
classas both a string and an object confirm that user-supplied classes are merged correctly with internal button classes and that conditional object entries behave as expected. This should prevent regressions around the newclassprop behavior and its interaction withuseMergeClasses.
170-193: Update disabled-link expectation to match component behavior and avoid relying ondisabled="false"
it('should handle disabled state for link elements correctly', ...)currently expects:const disabledAttr = wrapper.find('a').attributes('disabled') expect(disabledAttr).toBe('false')But the component now only binds
disabledwhenbuttonComponent === 'button', so anchors will not have adisabledattribute at all. Relying ondisabled="false"for links is also semantically odd.Consider changing this test to assert that no
disabledattribute is present and, if you add any visual/ARIA treatment for disabled links, assert that instead:- const disabledAttr = wrapper.find('a').attributes('disabled') - expect(disabledAttr).toBe('false') + const link = wrapper.find('a') + expect(link.attributes('disabled')).toBeUndefined() + // Optionally: expect(link.attributes('aria-disabled')).toBe('true') or similar, if implemented.This will bring the test suite back in sync with the component and align with standard HTML semantics.
src/components/FwbButton/FwbButton.vue (2)
133-157: Router fallback leaves<a>withouthrefwhen vue-router is missing; adjust linkAttr to keep navigation workingWhen
props.tois provided butrouter-linkis unavailable:
buttonComponentfalls back to'a'in thecatchbranch, butlinkAttrstill returns'to'andlinkValuereturnsprops.to.This produces markup like
<a to="/dashboard">...</a>, which won't navigate anywhere even though you attempted a graceful fallback. The warning helps, but the user-visible behavior is a non-functional link.Consider basing
linkAttron whetherrouter-linkcould actually be resolved, so that in the fallback case you emithrefinstead ofto:-const buttonComponent = computed(() => { - if (props.to) { - try { - return resolveComponent('router-link') - } catch { - console.warn('router-link component not found. Make sure vue-router is installed and properly configured.') - return 'a' - } - } - return props.href ? linkComponent : 'button' -}) +let resolvedRouterLink: any | null +let routerLinkResolved = false + +const resolveRouterLink = () => { + if (!routerLinkResolved) { + routerLinkResolved = true + try { + resolvedRouterLink = resolveComponent('router-link') + } catch { + resolvedRouterLink = null + } + } + return resolvedRouterLink +} + +const buttonComponent = computed(() => { + if (props.to) { + const routerLink = resolveRouterLink() + if (routerLink) return routerLink + + console.warn('router-link component not found. Make sure vue-router is installed and properly configured.') + return 'a' + } + return props.href ? linkComponent : 'button' +}) -const linkAttr = computed(() => { - if (props.to) return 'to' +const linkAttr = computed(() => { + if (props.to) { + // Use `to` only when router-link is actually available; otherwise fall back to `href`. + return resolveRouterLink() ? 'to' : 'href' + } if (props.href) return props.tag === 'router-link' || props.tag === 'nuxt-link' ? 'to' : 'href' return undefined })
linkValuecan stay as-is (it already returnsprops.to/props.href), and this keeps the existing precedence (tooverhref) while making the non-router fallback actually render a working<a href="...">.
2-7: Disabled behavior only applies to<button>elements; update tests and consider adding disabled enforcement for interactive linksThe binding
:disabled="buttonComponent === 'button' && disabled"correctly avoids invaliddisabledattributes on anchors/router-links. However:
- Non-button components (
<a>,<router-link>) will never receive adisabledattribute even whendisabledistrue, making disabled semantics invisible to users and assistive technologies.- Tests expecting
disabled='false'on link elements should be updated to reflect this implementation.For truly disabled link variants, add an accessible pattern:
aria-disabled="true"+pointer-events-none/cursor-not-allowedclass + early-return click handlers (e.g.,if (props.disabled) return). This ensures disabled state is enforced even when consumers attach click handlers, aligning with how other disabled components behave.
1 participant
Summary by CodeRabbit
New Features
toprop, with improved handling ofhrefanddisabledTests
✏️ Tip: You can customize this high-level summary in your review settings.