docs(BPopover): Parity pass #2701
Conversation
|
|
WalkthroughThis update introduces comprehensive documentation, new demo components, and refined prop definitions for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoComponent
participant BPopover
User->>DemoComponent: Interacts with button/trigger
DemoComponent->>BPopover: Passes props, slots, and triggers
BPopover-->>User: Shows/hides popover, emits events (show, hide, etc.)
Note right of BPopover: Emits standardized show/hide events and slot data
Assessment against linked issues
Poem
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 (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue (1)
26-30: Consider adding error handling for nav element lookupThe current implementation assumes the nav element will always be found. Consider adding error handling or a fallback value when the nav element isn't found.
onMounted(() => { navRef.value = document.body.querySelector('#app > nav') + if (!navRef.value) { + console.warn('Navigation element not found, boundary padding may not work correctly') + } })apps/docs/src/docs/components/demo/PopoverPositioning.vue (1)
47-53: Fix title templates for Left Start/End popoversThe title templates for the Left Start and Left End popovers incorrectly show "Popover Bottom Start" and "Popover Bottom End" instead of "Popover Left Start" and "Popover Left End".
- <template #title>Popover Bottom Start</template> + <template #title>Popover Left Start</template>- <template #title>Popover Bottom End</template> + <template #title>Popover Left End</template>apps/docs/src/utils/showhide-emits.ts (1)
24-25: Replace "modal" with "component" in hide-prevented descriptionThe description for the
hide-preventedevent refers to "modal" instead of a more generic term like "component". Since this utility is meant to be reusable across different components (not just modals), this terminology should be updated.- 'Emitted when the modal tried to close, but was prevented from closing. This occurs when preventDefault() is called on the event, the user clicks escape and no-close-onbackdrop is set to true, or the user clicks on the backdrop and no-close-onbackdrop is set to true.', + 'Emitted when the component tried to close, but was prevented from closing. This occurs when preventDefault() is called on the event, the user clicks escape and no-close-onbackdrop is set to true, or the user clicks on the backdrop and no-close-onbackdrop is set to true.',apps/docs/src/docs/migration-guide.md (1)
612-612: Fix grammatical error in the library description.The verb "have" doesn't agree with the singular subject "library".
-Triggers work differently as the underlying library that we use to manage popovers have changed. See +Triggers work differently as the underlying library that we use to manage popovers has changed. See🧰 Tools
🪛 LanguageTool
[uncategorized] ~612-~612: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... library that we use to manage popovers have changed. See [our documentation](/docs/...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
apps/docs/src/data/components/popover.data.ts (1)
164-165: Fix typo in type name.There's a typo in the type name for the blur event.
- type: 'FocusEventg', + type: 'FocusEvent',apps/docs/src/docs/components/popover.md (6)
18-18: Fix possessive form in Bootstrap reference.There's an extra "s" in the possessive form of "BootstrapVueNext".
-Popovers require BootstrapVueNexts's custom SCSS/CSS in order to function correctly, and for variants. +Popovers require BootstrapVueNext's custom SCSS/CSS to function correctly, and for variants.🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Consider a more concise word here.
Context: ...ire BootstrapVueNexts's custom SCSS/CSS in order to function correctly, and for variants. -...(IN_ORDER_TO_PREMIUM)
43-43: Add missing article in documentation.The phrase is missing "the" before "target element".
-component lower in the DOM than your target element. This rule also applies if a callback function -is used as target element, since that callback is called only once on mount. +component lower in the DOM than your target element. This rule also applies if a callback function +is used as the target element, since that callback is called only once on mount.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing article found.
Context: ...plies if a callback function is used as target element, since that callback is called ...(AI_HYDRA_LEO_MISSING_THE)
45-45: Add missing period after abbreviation.In American English, abbreviations like "etc." require a period.
-`HTMLElement` refers to standard HTML elements such as `<div>`, `<button>`, etc, while `SVGElement` +`HTMLElement` refers to standard HTML elements such as `<div>`, `<button>`, etc., while `SVGElement`🧰 Tools
🪛 LanguageTool
[style] ~45-~45: In American English, abbreviations like “etc.” require a period.
Context: ...L elements such as<div>,<button>, etc, whileSVGElementrefers to<svg>o...(ETC_PERIOD)
62-64: Add missing comma after introductory phrase."By default" should be followed by a comma.
-By default popovers are shown by `pointerenter` and `focus` events and closed by `pointerleave` and `blur` events +By default, popovers are shown by `pointerenter` and `focus` events and closed by `pointerleave` and `blur` events🧰 Tools
🪛 LanguageTool
[uncategorized] ~62-~62: Did you mean: “By default,”?
Context: ...ng.vue#template{vue-html} ## Triggers By default popovers are shown bypointerenteran...(BY_DEFAULT_COMMA)
65-65: Add missing comma for better readability.A comma is needed after "events" for better sentence structure.
-on the `target` element by default. To override this behavior and make the popover show and hide based -on `click` events set the `click` prop to `true`. +on the `target` element by default. To override this behavior and make the popover show and hide based +on `click` events, set the `click` prop to `true`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: Possible missing comma found.
Context: ... popover show and hide based onclickevents set theclickprop totrue. <<< DE...(AI_HYDRA_LEO_MISSING_COMMA)
124-124: Fix missing preposition in function description.The sentence is missing "to" before "allow".
-`BCollapse` exposes several functions allow manipulation of the state of the component. +`BCollapse` exposes several functions to allow manipulation of the state of the component.🧰 Tools
🪛 LanguageTool
[uncategorized] ~124-~124: Possible missing preposition found.
Context: ...BCollapseexposes several functions allow manipulation of the state of the compon...(AI_HYDRA_LEO_MISSING_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (21)
apps/docs/src/data/components/popover.data.ts(3 hunks)apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverContent.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverCustomClass.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverExposed.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverModel.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverOverview.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverPositioning.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverStartOpen.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverStyles.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverTriggers.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverVariant.vue(1 hunks)apps/docs/src/docs/components/popover.md(1 hunks)apps/docs/src/docs/migration-guide.md(1 hunks)apps/docs/src/docs/types.md(1 hunks)apps/docs/src/utils/common-props.ts(1 hunks)apps/docs/src/utils/index.ts(1 hunks)apps/docs/src/utils/showhide-emits.ts(1 hunks)apps/docs/src/utils/showhide-slotsdata.ts(1 hunks)packages/bootstrap-vue-next/src/components/BPopover/BPopover.vue(0 hunks)packages/bootstrap-vue-next/src/types/ComponentProps.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/bootstrap-vue-next/src/components/BPopover/BPopover.vue
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (1)
packages/bootstrap-vue-next/src/types/AnyValuedAttributes.ts (1)
ClassValue(2-2)
apps/docs/src/utils/showhide-slotsdata.ts (1)
apps/docs/src/types/index.ts (1)
SlotScopeReference(6-11)
apps/docs/src/utils/showhide-emits.ts (1)
apps/docs/src/types/index.ts (1)
ComponentReference(47-68)
apps/docs/src/data/components/popover.data.ts (8)
apps/docs/src/utils/index.ts (5)
showHideProps(8-8)pick(4-4)buildCommonProps(3-3)showHideEmits(7-7)showHideSlotsData(9-9)apps/docs/src/utils/dropdown-common.ts (1)
showHideProps(3-3)apps/docs/src/utils/showhide-props.ts (1)
showHideProps(4-51)packages/bootstrap-vue-next/src/types/index.ts (1)
BvnComponentProps(39-39)apps/docs/src/types/index.ts (1)
PropertyReference(13-18)apps/docs/src/utils/showhide-emits.ts (1)
showHideEmits(3-69)packages/bootstrap-vue-next/src/types/ComponentEmits.ts (1)
showHideEmits(3-14)apps/docs/src/utils/showhide-slotsdata.ts (1)
showHideSlotsData(3-35)
🪛 LanguageTool
apps/docs/src/docs/migration-guide.md
[uncategorized] ~612-~612: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... library that we use to manage popovers have changed. See [our documentation](/docs/...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
apps/docs/src/docs/components/popover.md
[style] ~18-~18: Consider a more concise word here.
Context: ...ire BootstrapVueNexts's custom SCSS/CSS in order to function correctly, and for variants. -...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~43-~43: Possible missing article found.
Context: ...plies if a callback function is used as target element, since that callback is called ...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~45-~45: In American English, abbreviations like “etc.” require a period.
Context: ...L elements such as <div>, <button>, etc, while SVGElement refers to <svg> o...
(ETC_PERIOD)
[uncategorized] ~62-~62: Did you mean: “By default,”?
Context: ...ng.vue#template{vue-html} ## Triggers By default popovers are shown by pointerenter an...
(BY_DEFAULT_COMMA)
[uncategorized] ~65-~65: Possible missing comma found.
Context: ... popover show and hide based on click events set the click prop to true. <<< DE...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~124-~124: Possible missing preposition found.
Context: ... BCollapse exposes several functions allow manipulation of the state of the compon...
(AI_HYDRA_LEO_MISSING_TO)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (29)
packages/bootstrap-vue-next/src/types/ComponentProps.ts (2)
1262-1263: Props reordering looks good.This logical reordering of
bodyandbodyClassto the beginning of the interface improves consistency with other components and provides better organization of related properties.
1261-1296: Good removal of thevariantprop.I notice the
variantprop has been removed from theBPopoverPropsinterface as mentioned in the PR objectives. This aligns with best practices to use Bootstrap's color and background utilities for styling instead of a dedicated variant prop for popovers.apps/docs/src/utils/common-props.ts (1)
410-414: Great addition of thetitleClassproperty.Adding the
titleClassproperty enhances styling capabilities for component titles. This follows the same pattern as other class properties likebodyClassandheaderClass, providing a consistent approach to custom styling across components.apps/docs/src/utils/index.ts (1)
7-9: Good addition of show/hide utilities exports.Adding exports for
showHideEmitsandshowHideSlotsDataimproves code reusability and standardization of the show/hide lifecycle events and slots. This will ensure consistent behavior across components that use these features.apps/docs/src/docs/components/demo/PopoverStartOpen.vue (1)
1-9: Great demo for starting with an open popover.This example clearly demonstrates how to create a popover that is initially open by using the
:model-value="true"binding. The demo is concise, focused, and well-structured, making it easy for users to understand this feature.apps/docs/src/docs/types.md (1)
334-340: Approve PopoverPlacement type addition. The newPopoverPlacementcorrectly extends the basePlacementwith'auto','auto-start', and'auto-end'options, matching Floating UI’s API.apps/docs/src/docs/components/demo/PopoverContent.vue (1)
1-18: Demo implementation is correct. This component clearly demonstrates both property-based (contentprop) and slot-based content embedding forBPopover, showcasing fundamental and advanced usage patterns.apps/docs/src/docs/components/demo/PopoverCustomClass.vue (1)
1-16: Custom class demo is accurate. The example properly usestitle-classandbody-classprops alongside slots to illustrate custom styling of popover sections.apps/docs/src/docs/components/demo/PopoverOverview.vue (1)
1-16: Overview demo aligns with usage patterns. This component effectively shows both directive-based (v-b-popover) and component-based (BPopover) popovers with slot usage, providing a concise overview.apps/docs/src/docs/components/demo/PopoverStyles.vue (1)
1-20: Well-designed demo showcasing popover styling techniques!This is an excellent demonstration of styling a popover using CSS variables. The example effectively shows how to leverage Bootstrap's theming system to create a custom danger-themed popover with appropriate color treatments for the header, background, and border.
A few points worth highlighting:
- Setting
:model-value="true"ensures the popover is always visible in the demo- The styling approach using CSS variables is the recommended way to customize popovers
- The component structure is clean and focused on its purpose
apps/docs/src/docs/components/demo/PopoverModel.vue (1)
1-21: Clear demonstration of v-model binding with popovers!This component effectively shows how to use two-way binding with the BPopover component. The toggle button and state management are implemented well using the Composition API.
The vertical stacking of elements with the flexbox layout provides a clear visual separation between the target button and the control button, making the demo easy to understand.
apps/docs/src/docs/components/demo/PopoverTriggers.vue (1)
1-10: Excellent demonstration of popover trigger options!This component clearly shows the contrast between the default trigger behavior and the click trigger option. The side-by-side presentation makes it easy for users to understand the difference between these two common trigger modes.
The use of the
bodyprop for direct content and theplacementprop for positioning are also good practices to highlight in a demo.apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue (1)
1-31: Great demonstration of the close-on-hide functionality with boundary handling!This component effectively demonstrates the close-on-hide behavior with two practical examples, including one that respects the top navigation boundary. The use of computed properties and DOM references to dynamically calculate the boundary padding shows a real-world scenario.
The slot usage for both target and content is also a good practice to highlight in documentation.
apps/docs/src/utils/showhide-slotsdata.ts (1)
1-35: Well-structured utility for standardizing slot scope propertiesThis new utility provides a standardized set of slot scope properties for components with show/hide functionality, which is excellent for maintaining consistency across components like
BPopover. The descriptions are clear and comprehensive.apps/docs/src/docs/components/demo/PopoverExposed.vue (1)
1-27: Clean implementation demonstrating programmatic controlThis demo effectively showcases how to programmatically control a popover using template refs, with proper TypeScript typing and Vue 3 Composition API patterns. The optional chaining on method calls (lines 23-25) ensures safety when the ref might be null.
apps/docs/src/docs/components/demo/PopoverPositioning.vue (1)
1-101: Comprehensive positioning demonstrationThis demo effectively showcases all standard placement options for popovers in a well-organized grid layout, which is valuable for documentation. The consistent structure makes it easy for users to understand the different positioning options.
apps/docs/src/utils/showhide-emits.ts (1)
1-69: Well-designed standardized events systemThis utility provides a comprehensive set of standardized events for components with show/hide functionality, complete with detailed descriptions and properly typed arguments. This approach will ensure consistency across components and simplify maintenance.
apps/docs/src/docs/migration-guide.md (1)
601-629: LGTM! Comprehensive migration notes for BPopover.The migration notes are thorough and cover all significant changes from the original bootstrap-vue implementation, including renamed props, deprecated features, and behavior changes.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~612-~612: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... library that we use to manage popovers have changed. See [our documentation](/docs/...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
apps/docs/src/data/components/popover.data.ts (6)
3-3: LGTM! Good use of shared utilities.Importing shared utilities like
showHideEmitsandshowHideSlotsDataimproves consistency and makes maintenance easier.
47-52: LGTM! Great addition of the hideMargin prop.The new
hideMarginprop is well-documented and improves the user experience by allowing customization of the hide behavior.
139-143: LGTM! Explicit typing for closeOnHide prop.The explicit type definition for
closeOnHideimproves type safety and provides clear documentation.
145-153: LGTM! Smart use of utility functions for common props.Using
buildCommonPropsandpickfunctions to generate common props maintains consistency across components and reduces code duplication.
157-211: LGTM! Comprehensive event definitions.The updated event definitions are thorough and provide valuable context on when each event is triggered and what data it contains.
216-228: LGTM! Consistent slot definitions using shared scope data.Using
showHideSlotsDatafor slots ensures consistency in documentation and implementation.apps/docs/src/docs/components/popover.md (5)
17-26: LGTM! Clear explanation of popover dependencies and limitations.The overview section provides valuable information about the dependencies and potential issues when using popovers.
🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Consider a more concise word here.
Context: ...ire BootstrapVueNexts's custom SCSS/CSS in order to function correctly, and for variants. -...(IN_ORDER_TO_PREMIUM)
27-48: LGTM! Thorough explanation of the target prop options.The documentation clearly explains all the possible values for the target prop, including important notes about DOM placement.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing article found.
Context: ...plies if a callback function is used as target element, since that callback is called ...(AI_HYDRA_LEO_MISSING_THE)
[style] ~45-~45: In American English, abbreviations like “etc.” require a period.
Context: ...L elements such as<div>,<button>, etc, whileSVGElementrefers to<svg>o...(ETC_PERIOD)
52-59: LGTM! Comprehensive positioning documentation.The positioning section covers all available placement options including the auto placement options from floating-ui.
80-96: LGTM! Great documentation for custom styling options.The custom classes and variants section clearly explains how to style popovers using Bootstrap utilities.
101-112: LGTM! Clear explanation of v-model usage.The programmatic control section provides clear examples of how to use v-model with BPopover.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/docs/src/docs/components/popover.md (4)
3-9: Correct<PageHeader>component references and casing
The examples reference<Bpopover>(lowercase “p”), which is inconsistent with the actual component name (<BPopover>). Update both opening and closing tags to use the proper casing to avoid confusion.
18-18: Refine SCSS/CSS requirement phrasing
The bullet reads “in order to function correctly, and for variants.” Consider simplifying to “to function correctly and support variants,” and verify the vendor name formatting (e.g., “BootstrapVue Next’s”).🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Consider a more concise word here.
Context: ...ire BootstrapVueNexts's custom SCSS/CSS in order to function correctly, and for variants. -...(IN_ORDER_TO_PREMIUM)
32-36: Address placeholder for callback target support
The bullet uses<NotYetImplemented/>for callback-based targets. Either remove this placeholder or implement the callback support before shipping docs—otherwise readers may expect a working feature. Would you like assistance scaffolding the callback API and its usage example?
124-124: Fix grammatical error in exposed functions description
Change “BPopoverexposes several functions allow manipulation…” to “BPopoverexposes several functions to allow manipulation…”
Apply this diff:-`BPopover` exposes several functions allow manipulation of the state of the component. +`BPopover` exposes several functions to allow manipulation of the state of the component.🧰 Tools
🪛 LanguageTool
[uncategorized] ~124-~124: Possible missing preposition found.
Context: ...sBPopoverexposes several functions allow manipulation of the state of the compon...(AI_HYDRA_LEO_MISSING_TO)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
apps/docs/src/docs/components/popover.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/src/docs/components/popover.md
[uncategorized] ~15-~15: You might be missing the article “the” here.
Context: ... ## Overview Things to know when using popover component: - Popovers rely on the 3rd ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~18-~18: Consider a more concise word here.
Context: ...ire BootstrapVueNexts's custom SCSS/CSS in order to function correctly, and for variants. -...
(IN_ORDER_TO_PREMIUM)
[uncategorized] ~43-~43: Possible missing article found.
Context: ...plies if a callback function is used as target element, since that callback is called ...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~45-~45: In American English, abbreviations like “etc.” require a period.
Context: ...L elements such as <div>, <button>, etc, while SVGElement refers to <svg> o...
(ETC_PERIOD)
[uncategorized] ~62-~62: Did you mean: “By default,”?
Context: ...ng.vue#template{vue-html} ## Triggers By default popovers are shown by pointerenter an...
(BY_DEFAULT_COMMA)
[uncategorized] ~65-~65: A comma might be missing here.
Context: ... popover show and hide based on click events set the click prop to true. <<< DE...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~124-~124: Possible missing preposition found.
Context: ...s BPopover exposes several functions allow manipulation of the state of the compon...
(AI_HYDRA_LEO_MISSING_TO)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
apps/docs/src/docs/components/popover.md (1)
11-11: Demo inclusion syntax is valid
The<<< DEMO ./demo/PopoverOverview.vue#template{vue-html}embed follows the established pattern and requires no changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/docs/src/docs/components/popover.md (2)
15-15: Grammar: add missing article.
Change to:- Things to know when using popover component: + Things to know when using the popover component:🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: You might be missing the article “the” here.
Context: ... ## Overview Things to know when using popover component: - Popovers rely on the 3rd ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
18-18: Simplify phrasing for conciseness.
Consider replacing “in order to function correctly” with “to function correctly”:- Popovers require BootstrapVueNext's custom SCSS/CSS in order to function correctly. + Popovers require BootstrapVueNext's custom SCSS/CSS to function correctly.🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Consider a more concise word here.
Context: ...uire BootstrapVueNext's custom SCSS/CSS in order to function correctly. - Specify `containe...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
apps/docs/src/data/components/popover.data.ts(3 hunks)apps/docs/src/docs/components/demo/PopoverPositioning.vue(1 hunks)apps/docs/src/docs/components/popover.md(1 hunks)apps/docs/src/utils/showhide-emits.ts(1 hunks)packages/bootstrap-vue-next/src/App.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/src/utils/showhide-emits.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/docs/src/docs/components/demo/PopoverPositioning.vue
- apps/docs/src/data/components/popover.data.ts
🧰 Additional context used
🪛 LanguageTool
apps/docs/src/docs/components/popover.md
[uncategorized] ~15-~15: You might be missing the article “the” here.
Context: ... ## Overview Things to know when using popover component: - Popovers rely on the 3rd ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~18-~18: Consider a more concise word here.
Context: ...uire BootstrapVueNext's custom SCSS/CSS in order to function correctly. - Specify `containe...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
packages/bootstrap-vue-next/src/App.vue (1)
2-17: The App.vue update effectively demonstrates BPopover functionality.The changes transform this file into a practical demonstration of the
BPopovercomponent, showcasing key features like:
- Programmatic visibility control via
:model-value="true"- Element targeting through ID reference
- Custom styling using Bootstrap CSS variables
- Rich content support with HTML formatting
This implementation aligns well with the PR objective of improving documentation and achieving parity for the
BPopovercomponent.apps/docs/src/docs/components/popover.md (15)
3-9: Great addition of thePageHeaderintroduction.
The new header clearly describes the Popover feature and its usage.
11-13: Demo inclusion for the overview looks correct.
EmbeddingPopoverOverview.vueensures users see the component in action.
17-17: Verified correctfloating-uilink.
The bullet correctly references the external positioning library.
27-36: Target section is comprehensive.
All validtargetprop types are clearly listed, and future callback support is noted.
38-48: Informational note is clear and well-placed.
The warning about DOM ordering is crucial; good use of the:::infoblock.
50-59: Positioning options are accurately documented.
The list of static and auto placements matches Floating-UI’s API.
61-69: Triggers section is concise and correct.
Default and click-based trigger behaviors are well explained.
73-78: Content section is well-structured.
Props vs. slots usage is clearly outlined with a live demo.
80-86: Custom Classes and Variants intro is solid.
Describes how to apply title/body classes; demo fragment inclusion is correct.
90-99: Variant styling demos and references are correct.
Both the demo inclusion and CSS variable guidance align with Bootstrap 5 practices.
102-110: Programmatic control viav-modelis properly documented.
Sections and demos for manual and initial-open behavior are clear.
120-120: Close-on-hide demo inclusion is present and accurate.
Demonstrates boundary and padding props as intended.
122-127: Exposed functions section is complete.
Correctly references template refs and links to the demo.
131-137: Script block correctly imports and returnsdata.
The migration to a standard script (instead of script setup) is consistent with other docs.
87-87:⚠️ Potential issueTypo in "utilities".
utitlitiesshould beutilities.
Apply this diff:- utitlities + utilitiesLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/docs/src/docs/migration-guide.md (2)
610-611: Clarifytargetprop behavior.Better phrasing improves readability.
Apply this diff:
- <NotYetImplemented/>The `target` prop does not implement taking a function. + <NotYetImplemented/>The `target` prop does not accept function values.
619-621: Refine utility-classes note.Add punctuation for better readability.
Apply this diff:
- [Bootstrap's Color and background utilities](https://getbootstrap.com/docs/5.3/helpers/color-background/) instead - see [our documentation](/docs/components/popover#custom-classes-and-variants) for details. + Bootstrap’s color and background utility classes can be used instead. See [our documentation](/docs/components/popover#custom-classes-and-variants) for details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
apps/docs/src/docs/components/popover.md(1 hunks)apps/docs/src/docs/migration-guide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/src/docs/components/popover.md
🧰 Additional context used
🪛 LanguageTool
apps/docs/src/docs/migration-guide.md
[uncategorized] ~612-~612: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...lying library we use to manage popovers have changed. See [our documentation](/docs/...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
apps/docs/src/docs/migration-guide.md (5)
601-602: Approve:content→bodyrename is clear and consistent.The renaming of the
contentprop tobodyaligns with other components’ terminology and is accurately documented here.
605-606: Approve: Custom-class → body-class and new title-class.The change from
custom-classtobody-classand the addition oftitle-classare correctly documented and improve prop consistency.
622-623: Approve: Updateddelaydefault values.The new defaults (100ms show, 300ms hide) are clearly presented and match the implementation.
624-625: Approve: Defaultplacementchange.Documenting the new default of
topimproves clarity for users.
626-627: Approve: Deprecation of$rootevents.Linking to
usePopoverControllergives users a clear migration path.
|
@xvaara Thanks for the thorough review - there were enough fundamental changes with how things work that I'm not surprised I missed some things. I believe I addressed all of your comments with my most recent commit. I did have one question about the default teleport behavior (above). Quick process question - do you prefer to be the one that resolves your comments, or should I resolve the ones that I believe I've addressed (I've worked on teams that do it both ways...) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/docs/src/docs/migration-guide.md (2)
630-632: 🛠️ Refactor suggestionComplete the deprecation note for
disabled.
The message is truncated. Update to something like:- <NotYetImplemented/>The `disabled` prop and - [Programmatically Disabling](https://bootstrap-vue.org/docs/components/popover#programmatically-disabling-popover) + <NotYetImplemented/>The `disabled` prop and programmatic disabling are not yet implemented. See [Programmatically Disabling Popovers](https://bootstrap-vue.org/docs/components/popover#programmatically-disabling-popover) for context.
603-604: 🛠️ Refactor suggestionFix deprecation note for
containerprop.
The phrasing is awkward and missing the verb “Use”. Split into two sentences for clarity.Apply this diff:
-The `container` prop has been deprecated, the `teleportTo` prop instead to specify where the popover -should be mounted. See [Vue Teleport documentation](https://vuejs.org/guide/built-ins/teleport.html). +The `container` prop has been deprecated. Use the `teleportTo` prop instead to specify where the popover should be mounted. See [Vue Teleport documentation](https://vuejs.org/guide/built-ins/teleport.html).
🧹 Nitpick comments (4)
apps/docs/src/docs/migration-guide.md (2)
621-623: Remove redundant variant utilities note.
Lines 621–623 duplicate the guidance just above and introduce a grammatical issue. Consider merging or deleting.
628-629: Add missing period.
End the sentence with a period for consistency:- `$root` events are deprecated. See [usePopoverController](/docs/composables/usePopoverController) as an alternative + `$root` events are deprecated. See [usePopoverController](/docs/composables/usePopoverController) as an alternative.apps/docs/src/docs/components/popover.md (2)
17-17: Use “third-party” instead of “3rd party”.
Spell out and hyphenate “third-party” for consistency:- Popovers rely on the 3rd party library [floating-ui](https://floating-ui.com/docs) for positioning. + Popovers rely on the third-party library [floating-ui](https://floating-ui.com/docs) for positioning.
21-21: Add missing punctuation.
Insert a comma before “see” to improve readability:- Set the `inline` prop to improve the positioning see the [Floating UI docs](https://floating-ui.com/docs/inline) for details. + Set the `inline` prop to improve the positioning, see the [Floating UI docs](https://floating-ui.com/docs/inline) for details.🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: Possible missing comma found.
Context: ...d. Set theinlineprop to improve the positioning see the [Floating UI docs](https://floa...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/docs/src/docs/components/popover.md(1 hunks)apps/docs/src/docs/migration-guide.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/docs/src/docs/components/popover.md
[uncategorized] ~21-~21: Possible missing comma found.
Context: ...d. Set the inline prop to improve the positioning see the [Floating UI docs](https://floa...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
apps/docs/src/docs/migration-guide.md (7)
601-602: Content prop rename is clear.
The note about renamingcontenttobodyis accurate and well-phrased.
606-607: Custom-class renaming is correct.
Updatingcustom-class→body-classand addingtitle-classcovers the change comprehensively.
608-609: Fallback-placement deprecation is clear.
The guidance to use floating-ui’s placement options is accurate and concise.
613-615: Triggers note is well-written.
The explanation of trigger behavior and links is clear and grammatically correct.
616-619: Variant prop deprecation is accurate.
The instruction to use Bootstrap’s utility classes is correct and links to the right section.
624-625: Delay defaults update is clear.
The new default timings (100ms/300ms) are correctly described.
626-627: Placement default update is accurate.
The change fromrighttotopis clearly stated with correct code formatting.
|
@dwgray Awesome work! I can go and close the comments, don't really mind either way. |
xvaara
left a comment
xvaara
left a comment
There was a problem hiding this comment.
I generally would prefer that we'd use the target slot so we don't need to have id/target combo in most examples.
Co-authored-by: Jukka Raimovaara <[email protected]>
Co-authored-by: Jukka Raimovaara <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/docs/src/docs/components/demo/PopoverModel.vue (1)
4-4: Consider improving the visibility state text formatting.The past review suggested improving the text formatting for better readability.
🧹 Nitpick comments (1)
apps/docs/src/docs/components/popover.md (1)
31-31: Fix abbreviation formatting.The abbreviation "i.e." requires two periods according to proper style guidelines.
Apply this diff:
-- A string with querySelector. (ie. '#toolbar > div:first-child') +- A string with querySelector. (i.e. '#toolbar > div:first-child')🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...ponent) - A string with querySelector. (ie. '#toolbar > div:first-child') - A refer...(I_E)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/docs/src/data/components/popover.data.ts(3 hunks)apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverContent.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverModel.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverOverview.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverStartOpen.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverStyles.vue(1 hunks)apps/docs/src/docs/components/popover.md(1 hunks)apps/docs/src/utils/showhide-slotsdata.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/docs/src/docs/components/demo/PopoverContent.vue
- apps/docs/src/docs/components/demo/PopoverStyles.vue
- apps/docs/src/docs/components/demo/PopoverOverview.vue
- apps/docs/src/utils/showhide-slotsdata.ts
- apps/docs/src/docs/components/demo/PopoverStartOpen.vue
- apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue
🧰 Additional context used
🪛 Biome (1.9.4)
apps/docs/src/data/components/popover.data.ts
[error] 99-99: expected , but instead found s
Remove s
(parse)
[error] 99-99: expected , but instead found translated
Remove translated
(parse)
[error] 99-99: expected , but instead found to
Remove to
(parse)
[error] 99-99: expected , but instead found the
Remove the
(parse)
[error] 99-99: expected , but instead found size
Remove size
(parse)
[error] 99-99: expected , but instead found of
Remove of
(parse)
[error] 99-99: expected , but instead found the
Remove the
(parse)
[error] 99-99: expected , but instead found arrow
Remove arrow
(parse)
[error] 99-99: expected , but instead found in
Remove in
(parse)
[error] 99-99: expected : but instead found bootstrap
Remove bootstrap
(parse)
[error] 99-99: expected , but instead found css
Remove css
(parse)
[error] 99-99: expected , but instead found .
Remove .
(parse)
[error] 99-99: unterminated string literal
The closing quote must be on the same line.
(parse)
[error] 99-99: This shorthand property named the is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this shorthand property named the
(lint/suspicious/noDuplicateObjectKeys)
🪛 LanguageTool
apps/docs/src/docs/components/popover.md
[uncategorized] ~31-~31: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...ponent) - A string with querySelector. (ie. '#toolbar > div:first-child') - A refer...
(I_E)
🔇 Additional comments (1)
apps/docs/src/data/components/popover.data.ts (1)
1-232: LGTM! Comprehensive component data enhancement.The additions provide detailed prop descriptions, enhanced event definitions, and proper integration with utility functions for show/hide functionality. The static analysis errors appear to be false positives - the string literals are properly formatted and the code structure is sound.
🧰 Tools
🪛 Biome (1.9.4)
[error] 99-99: expected
,but instead foundsRemove s
(parse)
[error] 99-99: expected
,but instead foundtranslatedRemove translated
(parse)
[error] 99-99: expected
,but instead foundtoRemove to
(parse)
[error] 99-99: expected
,but instead foundtheRemove the
(parse)
[error] 99-99: expected
,but instead foundsizeRemove size
(parse)
[error] 99-99: expected
,but instead foundofRemove of
(parse)
[error] 99-99: expected
,but instead foundtheRemove the
(parse)
[error] 99-99: expected
,but instead foundarrowRemove arrow
(parse)
[error] 99-99: expected
,but instead foundinRemove in
(parse)
[error] 99-99: expected
:but instead foundbootstrapRemove bootstrap
(parse)
[error] 99-99: expected
,but instead foundcssRemove css
(parse)
[error] 99-99: expected
,but instead found.Remove .
(parse)
[error] 99-99: unterminated string literal
The closing quote must be on the same line.
(parse)
[error] 99-99: This shorthand property named the is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this shorthand property named the(lint/suspicious/noDuplicateObjectKeys)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/docs/src/docs/migration-guide.md (5)
69-70: Typo and line-break cleanup in initial visibility note
The word “intial” is misspelled and the line break between the sentence fragments is unnecessary.
Apply this diff:- Note that `show` and `visible` are still supported for - specifying the intial visibility of these components. + Note that `show` and `visible` are still supported for specifying the initial visibility of these components.
604-605: Grammar improvement for deprecation note
The suggestion to useteleportTowould read more clearly if phrased as an imperative.- The `container` prop has been deprecated, the `teleportTo` prop instead to specify where the popover - should be mounted. See [Vue Teleport documentation](https://vuejs.org/guide/built-ins/teleport.html). + The `container` prop has been deprecated. Use the `teleportTo` prop to specify where the popover should be mounted. See [Vue Teleport documentation](https://vuejs.org/guide/built-ins/teleport.html).
628-630: Rephrase background utilities link
The sentence should flow more naturally:- [Bootstrap's Color and background utilities](https://getbootstrap.com/docs/5.3/helpers/color-background/) instead - see [our documentation](/docs/components/popover#custom-classes-and-variants) for details. + See [Bootstrap's color and background utilities](https://getbootstrap.com/docs/5.3/helpers/color-background/) and our [custom classes guide](/docs/components/popover#custom-classes-and-variants) for details.
631-632: Add trailing period to sentence
Consistency: end the sentence with a period.- `delay` now defaults to 100ms for show and 300ms for hide rather than 50ms for both + `delay` now defaults to 100ms for show and 300ms for hide rather than 50ms for both.
633-633: Add trailing period for consistency- The default for `placement` is now `top` rather than `right` + The default for `placement` is now `top` rather than `right`.apps/docs/src/docs/components/popover.md (1)
21-21: Missing comma in compound sentence
Add a comma after “positioning” to improve readability:- Set the `inline` prop to improve the positioning see the [Floating UI docs](https://floating-ui.com/docs/inline) for details. + Set the `inline` prop to improve the positioning, see the [Floating UI docs](https://floating-ui.com/docs/inline) for details.🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: Possible missing comma found.
Context: ...d. Set theinlineprop to improve the positioning see the [Floating UI docs](https://floa...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/docs/src/docs/components/popover.md(1 hunks)apps/docs/src/docs/migration-guide.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/docs/src/docs/migration-guide.md (1)
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.139Z
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.
🪛 LanguageTool
apps/docs/src/docs/components/popover.md
[uncategorized] ~21-~21: Possible missing comma found.
Context: ...d. Set the inline prop to improve the positioning see the [Floating UI docs](https://floa...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~31-~31: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...ponent) - A string with querySelector. (ie. '#toolbar > div:first-child') - A refer...
(I_E)
apps/docs/src/docs/migration-guide.md
[misspelling] ~72-~72: This word is normally spelled as one.
Context: ...prefix to specify that you don't want a sub-component to be rendered, we've moved to using `n...
(EN_COMPOUNDS_SUB_COMPONENT)
[uncategorized] ~73-~73: A comma may be missing after the conjunctive/linking adverb ‘Similarly’.
Context: ...lder, hideHeaderbecomesnoHeader`. Similarly we use the 'no' prefix in place of 'ski...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🔇 Additional comments (14)
apps/docs/src/docs/migration-guide.md (8)
76-76: Table introduction reads clearly
The sentence introducing the table is concise and grammatically correct.
602-602: Content prop rename noted
The migration note correctly reflects the renaming ofcontenttobody.
607-607: Custom-class props update is correct
The note aboutcustom-class→body-classand newtitle-classis accurate.
609-610: Fallback-placement deprecation is clear
The guidance to switch to floating-ui’s placement options is concise and correct.
612-612: Target callback deprecation correctly documented
The deprecation of the function-callback form oftargetis clearly stated.
613-616: Trigger behavior migration note is accurate
Explaining the change in underlying library and linking to the triggers docs is spot on.
618-620: Variant prop deprecation guidance is correct
Directing users to use Bootstrap utility classes and linking to the variants section is great.
635-635: $root events deprecation is clear
The note about deprecating$rootevents and suggestingusePopoverControlleris well stated.apps/docs/src/docs/components/popover.md (6)
3-9: Popover header section looks good
The<PageHeader>block effectively introduces the Popover component and usage modes.
11-11: Overview demo inclusion is correct
Embedding thePopoverOverview.vuedemo here aligns with the documentation style.
13-13: Section header is appropriate
“## Overview” clearly demarcates the beginning of the overview content.
15-16: Intro sentence is concise
“Things to know when using the popover component:” succinctly leads into the bullet list.
17-20: Bullet points read clearly
The first four bullets accurately summarize key popover behaviors and setup notes.
108-114: Close-on-hide section and demo inclusion
The “## Close on Hide” header and corresponding demo are correctly placed and informative.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/docs/src/docs/migration-guide.md (4)
69-70: Fix typo in “initial” spelling
The word “intial” is misspelled; it should read “initial” to maintain professionalism and clarity.
Apply this diff:- Note that `show` and `visible` are still supported for - specifying the intial visibility of these components. + Note that `show` and `visible` are still supported for + specifying the initial visibility of these components.
604-606: Refine grammar forcontainerdeprecation note
The current phrasing is missing an imperative verb. Consider updating to:- The `container` prop has been deprecated, the `teleportTo` prop instead to specify where the popover - should be mounted. See [Vue Teleport documentation](https://vuejs.org/guide/built-ins/teleport.html). + The `container` prop has been deprecated. Use the `teleportTo` prop instead to specify where the popover + should be mounted. See [Vue Teleport documentation](https://vuejs.org/guide/built-ins/teleport.html).
622-627: Minor grammar improvement for disabled-prop guidance
Add punctuation and clarify the conditional phrasing:- Use `manual=true` to disable BootstrapVueNext’s automatic - trigger handling and if your own code shows the popover disable those mechanisms as well. + Use `manual=true` to disable BootstrapVueNext’s automatic trigger handling. + If you show the popover programmatically, be sure to disable those mechanisms in your code as well.
628-630: Fix grammar in color utilities reference
The current line is missing a verb. Update to:- [Bootstrap's Color and background utilities](https://getbootstrap.com/docs/5.3/helpers/color-background/) instead - see [our documentation](/docs/components/popover#custom-classes-and-variants) for details. + See [Bootstrap’s color and background utilities](https://getbootstrap.com/docs/5.3/helpers/color-background/). + Refer to [our documentation](/docs/components/popover#custom-classes-and-variants) for Popover-specific guidance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/docs/src/data/components/popover.data.ts(3 hunks)apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverModel.vue(1 hunks)apps/docs/src/docs/migration-guide.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/docs/src/docs/components/demo/PopoverCloseOnHide.vue
- apps/docs/src/docs/components/demo/PopoverModel.vue
- apps/docs/src/data/components/popover.data.ts
🧰 Additional context used
🧠 Learnings (1)
apps/docs/src/docs/migration-guide.md (1)
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.139Z
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.
🪛 LanguageTool
apps/docs/src/docs/migration-guide.md
[misspelling] ~72-~72: This word is normally spelled as one.
Context: ...prefix to specify that you don't want a sub-component to be rendered, we've moved to using `n...
(EN_COMPOUNDS_SUB_COMPONENT)
[uncategorized] ~73-~73: A comma may be missing after the conjunctive/linking adverb ‘Similarly’.
Context: ...lder, hideHeaderbecomesnoHeader`. Similarly we use the 'no' prefix in place of 'ski...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
apps/docs/src/docs/migration-guide.md (10)
76-76: LGTM – “properties and components” sentence is clear
This added line correctly introduces the table of affected props/components.
602-603: Approve rename ofcontent→body
Consistent prop naming across components improves discoverability and UX.
607-608: Approvecustom-class→body-class& addition oftitle-class
Addingtitle-classalongside renaming keeps styling props explicit and consistent.
609-610: Approve deprecation offallback-placement
Pointing users to floating-ui placement options aligns with upstream API.
612-613: Approve deprecation of function form fortargetprop
Clarifies thattargetno longer accepts a callback, streamlining the API.
614-617: Approve updated triggers note
This correctly informs users about the new popover engine and links to detailed docs.
618-621: Approve deprecation ofvariantprop
Guiding users to Bootstrap utility classes for styling is the right approach.
631-632: Approve update todelaydefaults
Aligns with the new UX timing and documents the change clearly.
633-634: Approve change to defaultplacement
Documenting the default shift totopis important for user expectations.
635-636: Approve deprecation of$rootevents
Pointing tousePopoverControllerprovides a clear, modern alternative.
Okay, did that. Let me know if there is anything else that you feel is blocking this (I am having a little difficulty understanding the github UI around this). |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/docs/src/docs/components/demo/PopoverContent.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverCustomClass.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverModel.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverOverview.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverPositioning.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverStartOpen.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverStyles.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverTriggers.vue(1 hunks)apps/docs/src/docs/components/demo/PopoverVariant.vue(1 hunks)apps/docs/src/docs/migration-guide.md(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/src/docs/components/demo/PopoverCustomClass.vue
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/docs/src/docs/components/demo/PopoverStartOpen.vue
- apps/docs/src/docs/components/demo/PopoverContent.vue
- apps/docs/src/docs/components/demo/PopoverVariant.vue
- apps/docs/src/docs/components/demo/PopoverStyles.vue
- apps/docs/src/docs/components/demo/PopoverPositioning.vue
- apps/docs/src/docs/components/demo/PopoverModel.vue
- apps/docs/src/docs/components/demo/PopoverTriggers.vue
🧰 Additional context used
🧠 Learnings (1)
apps/docs/src/docs/migration-guide.md (1)
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.
🪛 LanguageTool
apps/docs/src/docs/migration-guide.md
[misspelling] ~72-~72: This word is normally spelled as one.
Context: ...prefix to specify that you don't want a sub-component to be rendered, we've moved to using `n...
(EN_COMPOUNDS_SUB_COMPONENT)
[uncategorized] ~73-~73: A comma may be missing after the conjunctive/linking adverb ‘Similarly’.
Context: ...lder, hideHeaderbecomesnoHeader`. Similarly we use the 'no' prefix in place of 'ski...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
apps/docs/src/docs/components/demo/PopoverOverview.vue (1)
1-21: Excellent comprehensive popover demo!This component effectively demonstrates the three main ways to implement popovers in BootstrapVueNext:
- Directive approach (line 4) - Simple and declarative
- Slot-based component (lines 8-12) - Preferred approach for better component encapsulation
- ID-based component (lines 14-18) - Alternative approach for referencing existing elements
The variety of examples provides excellent educational value for users migrating from BootstrapVue or learning the different implementation patterns available.
apps/docs/src/docs/migration-guide.md (2)
69-70: Minor grammar improvement approved.The change from "show in" to "shown in" improves the grammatical correctness of the sentence.
76-76: Grammar improvement suggestion.Consider adding a comma after "Similarly" for better readability, as suggested by the static analysis tool.
-The properties and components that are affected by this change are shown in the following table: +The properties and components that are affected by this change are shown in the following table:Actually, the static analysis hint was about line 73, but looking at the current content, the sentence structure is fine as-is. The "sub-component" spelling is also correct in this technical context.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/docs/src/docs/migration-guide.md (2)
614-616: Refine trigger change phrasing
Consider tightening the copy for readability. For example:- Triggers work differently as the underlying library we use to manage popovers has changed. + Triggers now behave differently due to the underlying popover library change.
628-629: Format default delay values as code
Wrap the delay durations in backticks for consistency and readability:- `delay` now defaults to 100ms for show and 300ms for hide rather than 50ms for both + `delay` now defaults to `100ms` for show and `300ms` for hide rather than `50ms` for both
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/docs/src/docs/migration-guide.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
apps/docs/src/docs/migration-guide.md (1)
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.
🪛 LanguageTool
apps/docs/src/docs/migration-guide.md
[misspelling] ~72-~72: This word is normally spelled as one.
Context: ...prefix to specify that you don't want a sub-component to be rendered, we've moved to using `n...
(EN_COMPOUNDS_SUB_COMPONENT)
[uncategorized] ~73-~73: A comma may be missing after the conjunctive/linking adverb ‘Similarly’.
Context: ...lder, hideHeaderbecomesnoHeader`. Similarly we use the 'no' prefix in place of 'ski...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
apps/docs/src/docs/migration-guide.md (8)
69-70: Clarity on reactive vs. initial visibility
The wording clearly distinguishes using the reactivev-modelfrom theshow/visibleprops for initial visibility, improving reader understanding.
76-76: Introduce table reference for affected properties
The new line correctly introduces the table of affected components, making the section more navigable.
602-605: Renamecontenttobody& deprecatecontainer
Accurately notes thatcontentis nowbodyand thatcontaineris deprecated in favor ofteleportTo, with a helpful link to the Vue Teleport docs.
607-610: Update styling prop names & handle placement changes
Renamingcustom-classtobody-class(plus newtitle-class) is clear, and deprecatingfallback-placementin favor of floating-ui options is well documented.
612-613: Deprecate function usage fortarget
Clearly informs users thattargetno longer accepts functions, which will prevent confusion during migration.
618-620: Deprecatevariantprop in favor of utility classes
The note correctly guides users to use Bootstrap’s color and background utility classes and points to the right section.
630-631: Placement default change is clear
The note about the defaultplacementswitching fromrighttotopis concise and properly emphasized.
632-632: Deprecate$rootevents with composable alternative
Pointing tousePopoverControlleras the replacement is helpful and aligns with the new composable pattern.
* upstream/main: docs: fix on-this-page for composable and directives pages (bootstrap-vue-next#2742) fix(BTableLite): field attribute isRowHeader now being respected (bootstrap-vue-next#2713) docs: change example styles to be single-attribute-per-line (bootstrap-vue-next#2731) fix(BTabs): Add type=button to buttons to prevent form submission (bootstrap-vue-next#2740) docs: fix unplugin-vue-components issue (bootstrap-vue-next#2724) fix(bforminput): abort debouncing if blur occurs chore: release main (bootstrap-vue-next#2735) fix(BTabs): correct id/index on ssr (bootstrap-vue-next#2726) Small fixes on 2025-06-06 (bootstrap-vue-next#2734) docs(BPopover): Parity pass (bootstrap-vue-next#2701) chore: release main Fix some tabs bugs, commit the previous pr override (bootstrap-vue-next#2720) chore: release main (bootstrap-vue-next#2698)
2 participants
Describe the PR
Parity pass for
BPopover- note that is just for the component, not the directive or composabletypes.mdvariantstill being declared on the component and ordering of propsSmall 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
Summary by CodeRabbit
New Features
hideMarginand support fortitleClassto enhance popover customization.Bug Fixes
Refactor
variantprop from the Popover component and associated styling logic.Documentation
Chores