-
-
Notifications
You must be signed in to change notification settings - Fork 163
feat(BDropdown): allow setting icon prop on nested BButton #2746
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
Conversation
|
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BDropdown
participant BButton
User->>BDropdown: Render with or without icon prop
BDropdown->>BButton: Pass icon prop (true/false)
BButton-->>User: Render button with icon attribute accordingly
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
1198-1206
:icon
prop correctly exposed but missing JSDoc / comment explaining intentThe new optional
icon?: boolean
flag forBDropdownProps
is perfectly fine from a type-safety standpoint.
Consider adding a one-liner JSDoc (similar tonoCaret
,noShift
, …) so that IDE tooltips and generated documentation clearly tell users that the value is forwarded to the nestedBButton
and triggers the.icon-link
styling.packages/bootstrap-vue-next/src/components/BDropdown/BDropdown.vue (1)
19-23
: Forwardingicon
unconditionally may produce unwanted styling on caret buttons
props.icon
is forwarded to the firstBButton
regardless ofsplit
mode.
Whensplit === false
, that first button is the toggle button with the caret. Enablingicon
in that case adds the.icon-link
class, potentially clashing with the caret styling and producing a visual artefact.- :icon="props.icon" + :icon="!props.split ? false : props.icon"This keeps the current behaviour for split buttons (where the first button is content-only) while avoiding accidental styling on plain toggles.
If you intentionally want the.icon-link
class on the toggle button, feel free to ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bootstrap-vue-next/src/components/BDropdown/BDropdown.vue
(2 hunks)packages/bootstrap-vue-next/src/components/BDropdown/dropdown.spec.ts
(1 hunks)packages/bootstrap-vue-next/src/types/ComponentProps.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test-unit
- GitHub Check: test-lint
- GitHub Check: build
🔇 Additional comments (2)
packages/bootstrap-vue-next/src/components/BDropdown/BDropdown.vue (1)
118-121
: Prop default wired up correctly – niceThe new
icon
default (false
) is set in_props
, guaranteeing deterministic behaviour even whenuseDefaults
overrides are present.packages/bootstrap-vue-next/src/components/BDropdown/dropdown.spec.ts (1)
257-270
: Tests cover default & truthy cases – good additionBoth edge cases (
false
by default and explicittrue
) are asserted, preventing regressions.
No further action required.
commit: |
Add the default |
@xvaara thanks, done |
* upstream/main: docs: clean up css selector docs: fix on-this-page when examples use header tags docs(migration): add component aliases guidelines (bootstrap-vue-next#2771) chore: upgrade dependencies and address all lint warnings (bootstrap-vue-next#2785) chore: release main (bootstrap-vue-next#2769) fix(BDropdown): prevent hydration warning in nuxt production mode (bootstrap-vue-next#2768) docs(BTabs): Updates based on v-model changes (bootstrap-vue-next#2760) docs(table): fix missing anchor in `BTableLite` and `BTableSimple` links (bootstrap-vue-next#2759) docs(BFormRating): Parity pass (bootstrap-vue-next#2749) docs: fix typo in breadcrumb documentation (bootstrap-vue-next#2756) docs: Fix empty-text and empty-filtered-text description as they require show-empty to be set (bootstrap-vue-next#2755) fix InputGroupXPend.vue chore: release main (bootstrap-vue-next#2748) feat: implement BFormRating component (bootstrap-vue-next#2744) chore: release main fix(BLink): move active class to BNavItem (bootstrap-vue-next#2747) feat(BDropdown): allow setting icon prop on nested BButton (bootstrap-vue-next#2746) fix(BToast): close BToast correctly if modelValue is changed from number to false (bootstrap-vue-next#2745)
Describe the PR
BLink and BButton have an
icon
prop which adds anicon-link
class onto the element, enhancing their appearance when an icon is present (docs). BDropdown (and components that extend it, such as BNavItemDropdown) wraps a BButton but provides no way to set the icon prop on it nor add custom classes. This PR adds theicon
prop to BDropdown (and by extension BNavItemDropdown), allowing the icon prop to be set on the nested BButton.Small replication
Before:

After:

PR checklist
What kind of change does this PR introduce? (check at least one)
fix(...)
feat(...)
fix(...)
docs(...)
The PR fulfills these requirements:
CHANGELOG
is generated from these messages, and determines the next version type. Pull requests that do not follow conventional commits or do not have an override will be deniedSummary by CodeRabbit
New Features
Tests
Documentation