-
-
Notifications
You must be signed in to change notification settings - Fork 174
feat(BPopover): add explicit props for focus and hover triggers #2795
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
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughThis change refactors the popover trigger system to allow independent control over click, focus, and hover triggers via separate boolean props. It updates component logic, types, documentation, and tests to support and demonstrate this granular trigger configuration. The tooltip component and related documentation are also adjusted for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TargetElement
participant BPopover
User->>TargetElement: Interacts (click, focus, hover)
TargetElement->>BPopover: Event triggered (if enabled)
alt Manual mode
BPopover-->>User: No automatic show/hide
else Triggers enabled
alt Click enabled
BPopover-->>User: Show/hide on click
end
alt Hover enabled
BPopover-->>User: Show/hide on hover
end
alt Focus enabled
BPopover-->>User: Show/hide on focus
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
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.
Pull Request Overview
This PR adds explicit control over individual popover triggers by introducing new focus and hover boolean props to complement the existing click prop. This provides fine-grained control over how popovers are triggered while maintaining backward compatibility.
- Introduces dedicated
focusandhoverprops for explicit trigger control - Updates trigger logic with priority system: manual mode > explicit props > click compatibility > default behavior
- Enhances documentation and demos to showcase the new trigger options
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/bootstrap-vue-next/src/types/ComponentProps.ts |
Adds focus and hover props to BPopoverProps interface |
packages/bootstrap-vue-next/src/components/BTooltip/BTooltip.vue |
Adds defaults for new trigger props in tooltip component |
packages/bootstrap-vue-next/src/components/BPopover/popover.spec.ts |
Comprehensive test coverage for new trigger behaviors and combinations |
packages/bootstrap-vue-next/src/components/BPopover/BPopover.vue |
Core implementation of new trigger logic with computed configuration |
apps/docs/src/docs/components/tooltip.md |
Minor formatting improvement in tooltip documentation |
apps/docs/src/docs/components/popover.md |
Expanded documentation explaining new trigger system and logic |
apps/docs/src/docs/components/demo/PopoverTriggers.vue |
Enhanced demo showcasing all trigger combinations |
apps/docs/src/data/components/popover.data.ts |
Updated prop documentation for new trigger props |
dwgray
left a comment
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.
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)
apps/docs/src/docs/components/popover.md (2)
60-65: Use canonical Vue Boolean-prop syntax in examplesIn Vue, a Boolean prop is considered
truewhen the attribute is present (e.g.<BPopover click>).
Writingclick="true"works, but it’s not idiomatic and can mislead users into quoting every Boolean.
Recommend updating the examples to the canonical form or to:click="true"when dynamic.- - **Click only**: Set `click="true"` - - **Hover only**: Set `hover="true"` - - **Focus only**: Set `focus="true"` - - **Multiple triggers**: Combine triggers like `click="true"` `hover="true"` `focus="true"` + - **Click only**: Add the `click` prop (`<BPopover click>`) + - **Hover only**: Add the `hover` prop (`<BPopover hover>`) + - **Focus only**: Add the `focus` prop (`<BPopover focus>`) + - **Multiple triggers**: Combine props, e.g. `<BPopover click hover focus>`
69-76: Clarify “explicit configuration” wordingThe current wording could be read as if setting either
hoverorfocussuppresses the other by default, but the actual logic treats each prop independently (undefined→ default,true/false→ explicit). Consider making the sentence explicit, e.g.:“Props that are explicitly set (
trueorfalse) override their respective defaults, while those leftundefinedfall through to the next rule.”This prevents confusion for users trying to disable one trigger but keep the other default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/docs/src/docs/components/popover.md(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.
Learnt from: dwgray
PR: bootstrap-vue-next/bootstrap-vue-next#2762
File: apps/docs/src/docs/components/tooltip.md:0-0
Timestamp: 2025-06-26T19:46:19.333Z
Learning: BTooltip is a very thin wrapper around BPopover in bootstrap-vue-next. There is no separate `useTooltipController` composable - the `usePopoverController` composable can be used to programmatically control both popovers and tooltips.
Learnt from: dwgray
PR: bootstrap-vue-next/bootstrap-vue-next#2701
File: apps/docs/src/docs/migration-guide.md:630-632
Timestamp: 2025-05-23T23:58:07.165Z
Learning: The `<NotYetImplemented/>` component in the bootstrap-vue-next documentation automatically renders text indicating "Not Yet Implemented", so additional explanatory text about features not being implemented is redundant when this component is used.
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2701
File: apps/docs/src/docs/migration-guide.md:622-626
Timestamp: 2025-06-05T11:43:10.793Z
Learning: In migration guides, links to the old/previous version's documentation (like bootstrap-vue.org) are appropriate and helpful when explaining deprecated features, as they provide users with reference points for what they're migrating from.
📚 Learning: btooltip is a very thin wrapper around bpopover in bootstrap-vue-next. there is no separate `usetool...
Learnt from: dwgray
PR: bootstrap-vue-next/bootstrap-vue-next#2762
File: apps/docs/src/docs/components/tooltip.md:0-0
Timestamp: 2025-06-26T19:46:19.333Z
Learning: BTooltip is a very thin wrapper around BPopover in bootstrap-vue-next. There is no separate `useTooltipController` composable - the `usePopoverController` composable can be used to programmatically control both popovers and tooltips.
Applied to files:
apps/docs/src/docs/components/popover.md
📚 Learning: in bootstrap-vue-next, `popoverorchestratorparam` includes an `id` field of type `controllerkey` thr...
Learnt from: xvaara
PR: bootstrap-vue-next/bootstrap-vue-next#2425
File: packages/bootstrap-vue-next/src/plugins/popoverController/index.ts:138-140
Timestamp: 2025-04-28T22:48:46.738Z
Learning: In bootstrap-vue-next, `PopoverOrchestratorParam` includes an `id` field of type `ControllerKey` through inheritance from base types. This field is declared in the `BvControllerOptions` interface and propagated through the type hierarchy.
Applied to files:
apps/docs/src/docs/components/popover.md
🪛 markdownlint-cli2 (0.17.2)
apps/docs/src/docs/components/popover.md
78-78: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
apps/docs/src/docs/components/popover.md (1)
77-79: Remove the double blank line to satisfy markdown-lint (MD012)There are two consecutive blank lines after the ordered list which triggers MD012.
-4. **Default behavior**: Both hover and focus triggers are active - - +4. **Default behavior**: Both hover and focus triggers are active
…keover * upstream/main: (21 commits) feat(b-form-rating): add `icon-clear` slot (#2809) docs: fix dead links, enable rule (#2808) chore: release main (#2801) docs: Fix navbar collapse behavior (#2802) docs: fix code tabs on getting started and icons pages (#2805) docs: fix missing data and use a stronger schema to catch missing errors (#2806) feat(BPopover): add explicit props for focus and hover triggers (#2795) fix(BNavbar): changed autoClose to noAutoClose and fix the documention mistake about it. fix: ssr in scrollspy docs: clean up css selector docs: fix on-this-page when examples use header tags docs(migration): add component aliases guidelines (#2771) chore: upgrade dependencies and address all lint warnings (#2785) chore: release main (#2769) fix(BDropdown): prevent hydration warning in nuxt production mode (#2768) docs(BTabs): Updates based on v-model changes (#2760) docs(table): fix missing anchor in `BTableLite` and `BTableSimple` links (#2759) docs(BFormRating): Parity pass (#2749) docs: fix typo in breadcrumb documentation (#2756) docs: Fix empty-text and empty-filtered-text description as they require show-empty to be set (#2755) ...
Describe the PR
closes #2783
A clear and concise description of what the pull request does.
Small replication
A small replication or video walkthrough can help demonstrate the changes made. This is optional, but can help observe the intended changes. A mentioned issue that contains a replication also works.
PR checklist
What kind of change does this PR introduce? (check at least one)
fix(...)feat(...)fix(...)docs(...)The PR fulfills these requirements:
CHANGELOGis 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
Documentation
Tests
Style