feat(BModal): use css var for zindex, add helper vars and ontop class #2556
Conversation
|
|
commit: |
|
demo on how to use: https://stackblitz.com/edit/github-etfudpj2-rwoeskjl?file=src%2FApp.vue |
|
I was thinking about this. We need a few more css variables in the code. Maybe we should import them from bootstrap and add to the :root element in our scss. toast width in toast orchestrator is at least another one. |
Sounds like a good addition |
That brings up something that I was looking at recently - in our getting started example, we directly import the generated .css files for bootstrap and bsvn. In the equivalent BSV example, the scss is imported. If we want developers to change our scss variables they'd need to import the SCSS. But if this is purely about CSS variables and we are moving to customization that way, we wouldn't. If that's the case, maybe we should be more explicit about moving to CSS variables? A section in the migration guide? |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a feature to use a CSS variable for the modal's z-index in the BModal component, adding helper variables and an "ontop" class to manage stacking order.
- Introduces an "ontop" class in both modal and backdrop elements.
- Updates the default z-index value by reading the CSS variable "--bs-modal-zindex" on component mount.
- Enhances computed z-index values with new helper CSS properties "--b-position" and "--b-count".
Comments suppressed due to low confidence (1)
packages/bootstrap-vue-next/src/components/BModal/BModal.vue:19
- [nitpick] Consider renaming the 'ontop' class to a more descriptive name such as 'modal-ontop' to improve clarity.
ontop: (activeModalCount ?? 0) - 1 === (activePosition ?? 0),
|
""" WalkthroughThe changes introduce a reactive approach to managing the z-index and stacking context for the modal component. Modal and backdrop elements now reflect their stack position via dynamic CSS classes and variables. The default z-index is determined at runtime using a new utility function that reads from a CSS variable, and backdrop click handling is restricted to left mouse button events. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BModal
participant ModalManager
participant DOMUtils
User->>BModal: Open Modal
BModal->>ModalManager: Get activePosition, activeModalCount
BModal->>DOMUtils: getModalZIndex(element)
DOMUtils-->>BModal: Return z-index value
BModal->>BModal: Compute sharedClasses and CSS vars
User->>BModal: Mousedown on backdrop (left button)
BModal->>BModal: Handle close if left button
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 8000 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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/bootstrap-vue-next/src/styles/styles.scss (1)
71-75: Expose-to-root is handy, but mind the global namespace & theming storyPlacing
--bs-modal-zindexand--bs-toast-max-widthon:rootmakes them globally mutable (👍), yet:
- Bootstrap 5 already defines a handful of
--bs-…vars in component scopes. Putting ours at:rootrisks accidental clashes/overrides coming from upstream versions or user stylesheets.- Apps that ship multiple themes (dark/light) often rely on
@media (prefers-color-scheme)or theme-specific classes. Consider scoping the declaration inside a theme selector or at least documenting that overriding must happen after this rule.- If the SCSS variables
$zindex-modalor$toast-max-widthare ever customised per-theme, emitting them once at root will freeze the first encountered value.You might prefer:
:root, [data-bv-theme='default'] { --bv-modal-zindex: #{$zindex-modal}; // avoid collision by changing prefix --bv-toast-max-width: #{$toast-max-width}; }(or similar) and update the consuming code accordingly.
packages/bootstrap-vue-next/src/components/BModal/BModal.vue (2)
377-381: Minor ordering concern: computed uses refs declared later
sharedClassescapturesactivePosition/activeModalCountbefore they are declared (they’re introduced a few lines below). Because the getter runs lazily this won’t throw, but it can surprise future maintainers. Moving theuseModalManagercall above this computed—or grouping related declarations—improves readability.
396-410:watch()insideonMounted()is safe but needlessly nestedThe watch only depends on
renderRefandelement, both mounted-only values, so nesting works—but you can register it once at module scope and checkelement.valueinside the callback, which avoids an extra closure:-watch( - renderRef, - (v) => { - if (!v) return - nextTick(() => { - if (!element.value) return - defaultModalDialogZIndex.value = getModalZIndex(element.value) - }) - }, - {immediate: true} -) +watch( + renderRef, + async (v) => { + if (!v) return + await nextTick() + if (element.value) + defaultModalDialogZIndex.value = getModalZIndex(element.value) + }, + {immediate: true} +)Eliminates the
onMountedwrapper and keeps reactivity setup in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bootstrap-vue-next/src/components/BModal/BModal.vue(8 hunks)packages/bootstrap-vue-next/src/styles/styles.scss(1 hunks)packages/bootstrap-vue-next/src/utils/dom.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/bootstrap-vue-next/src/components/BModal/BModal.vue (1)
28-29:❓ Verification inconclusive
@mousedown.leftmay not fire on touch devices—validate UXLimiting the backdrop handler to the left mouse button is sensible for desktop right-click menus, but touchscreens dispatch
pointerdown/touchstart, notmousedown. The previous.selfhandler incidentally worked everywhere; the new filter might stop modals from closing on tap.Please verify on mobile:
If touch support regresses, consider:
@pointerdown.self="hide('backdrop')"or keeping
.selfonly.
🏁 Script executed:
# quick grep to see if any other components rely on .left.mousedown rg "@mousedown\.left" -nLength of output: 138
Ensure backdrop taps close modal on touch devices
We’ve confirmed that the only occurrence of
@mousedown.left.selfis in
packages/bootstrap-vue-next/src/components/BModal/BModal.vue (line 28). On touchscreens,mousedownmay not fire (touch events triggerpointerdown/touchstartinstead), so tapping the backdrop could stop closing the modal.Possible approaches:
• Replace with pointer events:@pointerdown.self="hide('backdrop')"(this covers mouse, pen, and touch)
• Drop the.leftfilter if you prefer sticking withmousedown:@mousedown.self="hide('backdrop')"• Or use click events, which generally fire on both mouse and touch:
@click.self="hide('backdrop')"Please verify behavior on a mobile device or touch-enabled emulator to ensure taps still close the modal.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bootstrap-vue-next/src/utils/dom.ts (1)
72-75: Consider simplifying the multi-line method chain.The static analysis tool suggests condensing the chained method calls into a single line for better readability.
- const raw = window - .getComputedStyle(target) - .getPropertyValue('--bs-modal-zindex') - .trim() + const raw = window.getComputedStyle(target).getPropertyValue('--bs-modal-zindex').trim()🧰 Tools
🪛 GitHub Check: test-lint
[warning] 72-72:
Replace···=·window⏎····.getComputedStyle(target)⏎····.getPropertyValue('--bs-modal-zindex')⏎····with=·window.getComputedStyle(target).getPropertyValue('--bs-modal-zindex')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bootstrap-vue-next/src/utils/dom.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: test-lint
packages/bootstrap-vue-next/src/utils/dom.ts
[warning] 72-72:
Replace ···=·window⏎····.getComputedStyle(target)⏎····.getPropertyValue('--bs-modal-zindex')⏎···· with =·window.getComputedStyle(target).getPropertyValue('--bs-modal-zindex')
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/bootstrap-vue-next/src/utils/dom.ts (1)
70-79: Great implementation of the z-index utility function!The function correctly implements the fetching of the CSS variable
--bs-modal-zindexwith proper fallback handling. I appreciate the robust implementation that:
- Accepts optional/nullable elements
- Uses document.body as a fallback
- Trims the string value to avoid whitespace issues
- Properly handles parsing errors with Number.isFinite
- Provides a sensible default (1055) when needed
This implementation fully addresses the previous feedback and will support the modal's reactive z-index functionality nicely.
🧰 Tools
🪛 GitHub Check: test-lint
[warning] 72-72:
Replace···=·window⏎····.getComputedStyle(target)⏎····.getPropertyValue('--bs-modal-zindex')⏎····with=·window.getComputedStyle(target).getPropertyValue('--bs-modal-zindex')
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a CSS custom property for modal z-index and helper variables & classes for modal stacking.
- Adds a utility (
getModalZIndex) to read--bs-modal-zindexwith a fallback. - Defines
--bs-modal-zindexand--bs-toast-max-widthon:root. - Updates
BModalto apply CSS vars, generate stacking classes, and restrict backdrop dismissal to left clicks.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/bootstrap-vue-next/src/utils/dom.ts | Added getModalZIndex to read CSS var or default to 1055. |
| packages/bootstrap-vue-next/src/styles/styles.scss | Defined root-level --bs-modal-zindex and --bs-toast-max-width. |
| packages/bootstrap-vue-next/src/components/BModal/BModal.vue | Switched to CSS var for z-index, added stacking classes, and updated event modifiers. |
Comments suppressed due to low confidence (2)
packages/bootstrap-vue-next/src/components/BModal/BModal.vue:122
- To fully restrict backdrop dismissal to left mouse clicks, add the
.leftmodifier to the click handler, e.g.,@click.left.self="hide('backdrop')".
@click="hide('backdrop')"
packages/bootstrap-vue-next/src/utils/dom.ts:70
- Add unit tests for
getModalZIndexto cover scenarios where the CSS variable is present, missing, and invalid to ensure correct fallback behavior.
export const getModalZIndex = (element?: Readonly<HTMLElement | null>): number => {
| @@ -66,3 +66,11 @@ export const sortSlotElementsByPosition = ( | |||
| if (position & Node.DOCUMENT_POSITION_PRECEDING) return 1 | |||
| return 0 | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] Consider adding a JSDoc comment to explain the purpose, parameters, return value, and fallback behavior of getModalZIndex.
| /** | |
| * Retrieves the z-index value for a modal from the `--bs-modal-zindex` CSS variable. | |
| * | |
| * @param {Readonly<HTMLElement | null>} [element] - The target element to retrieve the z-index from. Defaults to `document.body` if not provided. | |
| * @returns {number} The z-index value. Returns 1055 as a fallback if `window` is undefined or the CSS variable is not parsable. | |
| */ |
Copilot uses AI. Check for mistakes.
| '--b-position': activePosition?.value ?? 0, | ||
| '--b-inverse-position': (activeModalCount?.value ?? 1) - 1 - (activePosition?.value ?? 0), | ||
| '--b-count': activeModalCount?.value ?? 0, | ||
| })) | ||
| const computedZIndexBackdrop = computed<CSSProperties>(() => ({ | ||
| 'z-index': computedZIndexNumber.value - 1, | ||
| '--b-position': activePosition?.value ?? 0, | ||
| '--b-inverse-position': (activeModalCount?.value ?? 1) - 1 - (activePosition?.value ?? 0), | ||
| '--b-count': activeModalCount?.value ?? 0, |
There was a problem hiding this comment.
[nitpick] Consider extracting CSS custom property names (e.g. --b-position, --b-inverse-position, --b-count) into constants to avoid typos and keep CSS/JS in sync.
| '--b-position': activePosition?.value ?? 0, | |
| '--b-inverse-position': (activeModalCount?.value ?? 1) - 1 - (activePosition?.value ?? 0), | |
| '--b-count': activeModalCount?.value ?? 0, | |
| })) | |
| const computedZIndexBackdrop = computed<CSSProperties>(() => ({ | |
| 'z-index': computedZIndexNumber.value - 1, | |
| '--b-position': activePosition?.value ?? 0, | |
| '--b-inverse-position': (activeModalCount?.value ?? 1) - 1 - (activePosition?.value ?? 0), | |
| '--b-count': activeModalCount?.value ?? 0, | |
| [CSS_VAR_POSITION]: activePosition?.value ?? 0, | |
| [CSS_VAR_INVERSE_POSITION]: (activeModalCount?.value ?? 1) - 1 - (activePosition?.value ?? 0), | |
| [CSS_VAR_COUNT]: activeModalCount?.value ?? 0, | |
| })) | |
| const computedZIndexBackdrop = computed<CSSProperties>(() => ({ | |
| 'z-index': computedZIndexNumber.value - 1, | |
| [CSS_VAR_POSITION]: activePosition?.value ?? 0, | |
| [CSS_VAR_INVERSE_POSITION]: (activeModalCount?.value ?? 1) - 1 - (activePosition?.value ?? 0), | |
| [CSS_VAR_COUNT]: activeModalCount?.value ?? 0, |
Copilot uses AI. Check for mistakes.
| const defaultModalDialogZIndex = 1056 | ||
| const defaultModalDialogZIndex = ref(getModalZIndex(element.value ?? document.body)) | ||
|
|
||
| onMounted(() => { |
There was a problem hiding this comment.
[nitpick] The watcher on renderRef triggers a recalculation of the z-index on every render. Consider limiting the watch to CSS variable changes or debouncing to avoid unnecessary getComputedStyle calls.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/docs/src/docs/components/modal.md (3)
177-181: Fix typo and suggest showcasing the newontopclassThe button label reads “Open Fouth Modal” but should be “Open Fourth Modal” to avoid confusion.
Additionally, since this PR introduces anontopCSS class for BModal, consider adding a small example here to demonstrate how to use<BModal ontop …>to keep a modal above the rest.
200-203: Fix typo in the HTML snippetThe live-code snippet still says “Open Fouth Modal”. Please correct it to “Open Fourth Modal” so the example matches the intended label.
274-290: Scope the injected styles to this page/componentThe
<style>block at the bottom will apply globally to all.modalselectors. To prevent unintended styling elsewhere in the docs, addscoped(or an equivalent selector) to limit its effect.Example:
- <style> + <style scoped>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/docs/src/docs/components/modal.md(3 hunks)packages/bootstrap-vue-next/src/utils/dom.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bootstrap-vue-next/src/utils/dom.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
apps/docs/src/docs/components/modal.md (1)
271-271: State is wired up correctlyThe new
const nestedModal4 = ref(false)properly adds reactive state for the fourth modal. This aligns with the updated HTML examples and will correctly drive visibility.
* upstream/main: chore: remove blur from boverlay in cases where its not allowed by br… (bootstrap-vue-next#2712) feat(BTableLite): add `table-colgroup` slot (bootstrap-vue-next#2680) perf: use getter functions over computed in some cases feat(BModal): use css var for zindex, add helper vars and ontop class (bootstrap-vue-next#2556) feat(BFormTags): added feedbackAriaLive prop (bootstrap-vue-next#2696) fix(scss) moved all scss styles out of components (bootstrap-vue-next#2671) fix(BModal): ensure clicking inside and releasing outside does not close modal (bootstrap-vue-next#2703) (bootstrap-vue-next#2704) fix(BToast): race condition if using setInterval to update countdown doc data feat(BPopover): add titleClass and bodyClass, remove unneeded customClass prop since class is inherited to the same place feat(BToast): add noProgress prop, make progress show as default if modelValue is number. fix(useToastController): if using the deprecated show method the countdown didn't start.
closes #2402
Describe the PR
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