feature: add prop to set modal location to topleft, topright, bottoml… #566
Conversation
…eft, bottom right
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant URL as URL Query
participant MIM as MapsIndoorsMap
participant MT as MapTemplate
participant SB as Sidebar
participant MOD as Modal
Note right of MIM: Initialization
MIM->>MIM: Resolve modalLocation (URL param if supportsUrlParameters else prop/default)
alt URL param present
URL-->>MIM: modalLocation (query)
end
MIM->>MT: render with mapTemplateProps.modalLocation
MT->>MT: forward modalLocation to MapWrapper and Sidebar
MT->>SB: modalLocation
SB->>MOD: modalLocation (applied to each Modal)
MOD->>MOD: add CSS class ".modal.<location>" to container (topleft/topright/bottomleft/bottomright)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/map-template/src/components/Sidebar/Sidebar.jsx (1)
108-113: EXTERNALIDS modal missing modalLocation (regression in placement)All other modals receive
modalLocation. With base offsets removed, EXTERNALIDS will have no anchor and may render unpredictably.Apply:
- <Modal isOpen={currentAppView === appViews.EXTERNALIDS} key="EXTERNALIDS"> + <Modal isOpen={currentAppView === appViews.EXTERNALIDS} key="EXTERNALIDS" modalLocation={modalLocation}>
🧹 Nitpick comments (8)
packages/map-template/src/components/Sidebar/Modal/Modal.scss (1)
54-81: Corner classes: kiosk conflict, hardcoded offsets, and z-index regression
- The corner selectors will override
.modal--kiosk(same specificity, later in file). Result: kiosk modals won’t stay centered. Scope corner rules to non‑kiosk.- Replace magic
10px/100pxwith spacing tokens for consistency; also the 100px asymmetry looks accidental.- Switching to
--z-index-dropdownmay push the modal under overlayed UI; keep--z-index-overlayunless intentionally changed.Apply:
- &.topleft { - top: 10px; - left: 10px; + &:not(.modal--kiosk).topleft { + top: var(--spacing-medium); + left: var(--spacing-medium); right: auto; bottom: auto; - z-index: var(--z-index-dropdown); + z-index: var(--z-index-overlay); } - &.topright { - top: 10px; - right: 100px; + &:not(.modal--kiosk).topright { + top: var(--spacing-medium); + right: var(--spacing-medium); left: auto; bottom: auto; - z-index: var(--z-index-dropdown); + z-index: var(--z-index-overlay); } - &.bottomleft { - bottom: 10px; - left: 100px; + &:not(.modal--kiosk).bottomleft { + bottom: var(--spacing-medium); + left: var(--spacing-medium); top: auto; right: auto; - z-index: var(--z-index-dropdown); + z-index: var(--z-index-overlay); } - &.bottomright { - bottom: 10px; - right: 10px; + &:not(.modal--kiosk).bottomright { + bottom: var(--spacing-medium); + right: var(--spacing-medium); left: auto; top: auto; - z-index: var(--z-index-dropdown); + z-index: var(--z-index-overlay); }packages/map-template/src/components/Sidebar/Modal/Modal.jsx (2)
9-11: Constrain and document modalLocation
- Use
oneOfto prevent invalid class names.- Add JSDoc for
modalLocation.Apply:
Modal.propTypes = { children: PropTypes.node, - isOpen: PropTypes.bool, - modalLocation: PropTypes.string + isOpen: PropTypes.bool, + modalLocation: PropTypes.oneOf(['topleft','topright','bottomleft','bottomright']) }; @@ - function Modal({ children, isOpen, modalLocation }) { + /** + * @param {'topleft'|'topright'|'bottomleft'|'bottomright'} [props.modalLocation] + */ + function Modal({ children, isOpen, modalLocation }) {Also applies to: 23-23
36-49: Guard refs correctly in MutationObserver effect
if (!contentRef)never trips; checkcontentRef.currentandmodalRef.currentbefore use.Apply:
- useEffect(() => { - if (!contentRef) return; + useEffect(() => { + if (!contentRef?.current || !modalRef?.current) return; const observer = new MutationObserver(() => { - const contentHeight = contentRef.current.clientHeight; - const modalHeight = modalRef.current?.clientHeight; + const contentHeight = contentRef.current?.clientHeight ?? 0; + const modalHeight = modalRef.current?.clientHeight ?? 0; setFullHeight(contentHeight > modalHeight); });packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx (2)
40-42: Type and docs: enumerate allowed modalLocation valuesAlign PropTypes and JSDoc with the four supported tokens and exact spellings (no space).
Apply:
- mapboxMapStyle: PropTypes.string, - modalLocation: PropTypes.string, + mapboxMapStyle: PropTypes.string, + modalLocation: PropTypes.oneOf(['topleft','topright','bottomleft','bottomright']), @@ - * @param {string} [props.modalLocation] - Specifies where the modal renders - default to top left. other options include topright, bottomleft, or bottomright. + * @param {'topleft'|'topright'|'bottomleft'|'bottomright'} [props.modalLocation] - Corner where the modal renders. Default: 'topleft'.Also applies to: 80-81
140-141: Normalize query param valueLowercase once to tolerate case variants; optional: strip hyphen.
Apply:
- const modalLocationQueryParameter = queryStringParams.get('modalLocation'); + const modalLocationQueryParameter = queryStringParams.get('modalLocation')?.toLowerCase()?.replace('-', '');packages/map-template/src/components/MapTemplate/MapTemplate.jsx (1)
103-105: PropTypes/JSDoc tighten; optional default in signature
- Use
oneOfformodalLocationand correct JSDoc spelling.- Optionally default in the signature for resilience.
Apply:
- mapboxMapStyle: PropTypes.string, - modalLocation: PropTypes.string, + mapboxMapStyle: PropTypes.string, + modalLocation: PropTypes.oneOf(['topleft','topright','bottomleft','bottomright']), @@ - * @param {string} [props.modalLocation] - Specifies where the modal renders - default to top left. other options include topright, bottomleft, or bottomright. + * @param {'topleft'|'topright'|'bottomleft'|'bottomright'} [props.modalLocation] - Corner where the modal renders. Default: 'topleft'. @@ -function MapTemplate({ ... , mapboxMapStyle, modalLocation }) { +function MapTemplate({ ... , mapboxMapStyle, modalLocation = 'topleft' }) {Also applies to: 142-145
packages/map-template/src/components/Sidebar/Sidebar.jsx (2)
40-41: Docs mismatch with implementationJSDoc lists "centered" and omits "topleft/topright". Align with the four supported tokens.
Apply:
- * @param {string} props.modalLocation - The location of the modal in the sidebar. Can be "bottomright", "bottomleft", "centered". + * @param {'topleft'|'topright'|'bottomleft'|'bottomright'} props.modalLocation - Corner position for the modal.
15-24: Constrain modalLocation PropTypeUse an enum to prevent invalid values.
Apply:
- onRouteFinished: PropTypes.func, - modalLocation: PropTypes.string + onRouteFinished: PropTypes.func, + modalLocation: PropTypes.oneOf(['topleft','topright','bottomleft','bottomright'])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/map-template/src/App.jsx(1 hunks)packages/map-template/src/components/MapTemplate/MapTemplate.jsx(3 hunks)packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx(5 hunks)packages/map-template/src/components/Sidebar/Modal/Modal.jsx(3 hunks)packages/map-template/src/components/Sidebar/Modal/Modal.scss(1 hunks)packages/map-template/src/components/Sidebar/Sidebar.jsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/map-template/src/components/Sidebar/Modal/Modal.jsx (2)
packages/map-template/src/components/Sidebar/Sidebar.jsx (1)
kioskLocation(46-46)packages/map-template/src/components/LocationDetails/LocationDetails.jsx (1)
kioskLocation(87-87)
packages/map-template/src/components/Sidebar/Sidebar.jsx (1)
packages/map-template/src/components/MapTemplate/MapTemplate.jsx (1)
pushAppView(209-209)
🔇 Additional comments (2)
packages/map-template/src/App.jsx (1)
11-12: LGTM: call‑site threads modalLocationPassing
modalLocation='topright'exercises the new flow end‑to‑end.packages/map-template/src/components/MapTemplate/MapTemplate.jsx (1)
800-801: LGTM: prop is forwarded to SidebarProp threading looks correct.
| useAppTitle: false, | ||
| modalLocation: 'topleft', | ||
| }; |
There was a problem hiding this comment.
Default not applied: modalLocation can be undefined
defaultProps.modalLocation is declared but not used when neither prop nor URL param is set. This breaks positioning after base offsets were removed.
Apply:
- modalLocation: 'topleft',
+ modalLocation: 'topleft',
@@
- modalLocation: props.supportsUrlParameters && modalLocationQueryParameter ? modalLocationQueryParameter : props.modalLocation,
+ modalLocation: (props.supportsUrlParameters && modalLocationQueryParameter)
+ ? modalLocationQueryParameter
+ : (props.modalLocation || defaultProps.modalLocation),Also applies to: 185-186
🤖 Prompt for AI Agents
In packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx around
lines 103-105 (and similarly at 185-186), defaultProps.modalLocation is declared
but not applied when neither the prop nor the URL param is present; update the
code to select modalLocation with a clear precedence (prop value -> URL param ->
defaultProps.modalLocation) so the default is used when others are undefined
(implement via destructuring default, nullish coalescing, or explicit fallback),
and apply the same fix at the other occurrence on lines 185-186.
| return <div ref={modalRef} | ||
| className={`modal ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`} | ||
| className={`modal ${modalLocation} ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid "undefined" CSS class and skip location class in kiosk
Prevent rendering class="undefined" and avoid corner class when kiosk is active.
Apply:
- return <div ref={modalRef}
- className={`modal ${modalLocation} ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`}
- >
+ const locationClass = !kioskLocation && modalLocation ? modalLocation : '';
+ return <div ref={modalRef}
+ className={`modal ${locationClass} ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`}
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return <div ref={modalRef} | |
| className={`modal ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`} | |
| className={`modal ${modalLocation} ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`} | |
| > | |
| const locationClass = !kioskLocation && modalLocation ? modalLocation : ''; | |
| return <div ref={modalRef} | |
| className={`modal ${locationClass} ${isOpen ? 'modal--open' : ''} ${fullHeight ? 'modal--full' : ''} ${kioskLocation ? 'modal--kiosk' : ''}`} | |
| > |
🤖 Prompt for AI Agents
In packages/map-template/src/components/Sidebar/Modal/Modal.jsx around lines
51-53, the className string can include "undefined" when modalLocation is falsy
and still adds the corner/location class even when kioskLocation is true; update
the className construction to build classes conditionally (e.g., use an array of
class tokens and push modalLocation only if it's a non-empty string, push
'modal--kiosk' only when kioskLocation is truthy, and skip adding modalLocation
when kioskLocation is active), then join the array with spaces (or use
conditional expressions) so no "undefined" token is emitted and the location
class is omitted in kiosk mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx (1)
7-41: DeclaremodalLocationinpropTypes.New public prop isn’t reflected in the PropTypes contract, so downstream consumers lose type guidance. Please add it (e.g. as a
oneOfof the supported values).showMapMarkers: PropTypes.bool, mapboxMapStyle: PropTypes.string, - devicePosition: PropTypes.object + devicePosition: PropTypes.object, + modalLocation: PropTypes.oneOf(['topleft', 'topright', 'bottomleft', 'bottomright']) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/map-template/src/components/MapTemplate/MapTemplate.jsx(3 hunks)packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/map-template/src/components/MapTemplate/MapTemplate.jsx
🔇 Additional comments (1)
packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx (1)
186-186: Default modal location still isn’t applied.When neither URL param nor prop is provided, this leaves
modalLocationundefined, so the modal loses its positioning class after the base styles were removed. Please fall back to the default.- modalLocation: props.supportsUrlParameters && modalLocationQueryParameter ? modalLocationQueryParameter : props.modalLocation, + modalLocation: (props.supportsUrlParameters && modalLocationQueryParameter) + ? modalLocationQueryParameter + : (props.modalLocation ?? defaultProps.modalLocation),
2 participants
…eft, bottom right
Summary by CodeRabbit
New Features
Style