IBX-9170: AI Assistant #1385
IBX-9170: AI Assistant #1385
Conversation
GrabowskiM
force-pushed
the
ai-assistant
branch
from
b11091b to
b242305
Compare
GrabowskiM
force-pushed
the
ai-assistant
branch
from
42c2d1a to
60e8ef7
Compare
GrabowskiM
force-pushed
the
ai-assistant
branch
from
60e8ef7 to
28e8501
Compare
GrabowskiM
force-pushed
the
ai-assistant
branch
from
25325d8 to
990cbc0
Compare
| onClose: () => {}, | ||
| onItemClick: () => {}, | ||
| positionOffset: () => ({ x: 0, y: 0 }), | ||
| scrollContainer: window.document.body, |
There was a problem hiding this comment.
Could you use helper function getRootDOMElement from https://github.com/ibexa/admin-ui/blob/main/src/bundle/Resources/public/js/scripts/helpers/context.helper.js#L62 ?
| &__item-content { | ||
| position: relative; | ||
| display: flex; | ||
| align-items: center; |
| cursor: pointer; | ||
| padding: calculateRem(9px); | ||
| color: $ibexa-color-dark; | ||
| font-size: calculateRem(14px); |
There was a problem hiding this comment.
we have variable for font-size
| &:disabled, | ||
| &--disabled { | ||
| pointer-events: none; | ||
| cursor: not-allowed; |
There was a problem hiding this comment.
pointer-events: none and cursor: not-allowed, is it working together?
|
|
||
| useEffect(() => { | ||
| if (isDragging) { | ||
| rootDOMElement.addEventListener('mousemove', handleDragging); |
There was a problem hiding this comment.
| rootDOMElement.addEventListener('mousemove', handleDragging); | |
| rootDOMElement.addEventListener('mousemove', handleDragging, false); |
just for consistency
| return null; | ||
| } | ||
|
|
||
| const groupClassName = createCssClassNames({ |
There was a problem hiding this comment.
I'm not sure if we should use createCssClassNames because this it's only one class and it's not conditional
| @@ -0,0 +1,24 @@ | |||
| const createDynamicRoot = (contextDOMElement = window.document.body, id) => { | |||
There was a problem hiding this comment.
window.document.body you can get this object by function getRootDOMElement from https://github.com/ibexa/admin-ui/blob/main/src/bundle/Resources/public/js/scripts/helpers/context.helper.js. We will avoid possible problems in the future if this code is used outside DXP
| onClose(); | ||
| }; | ||
|
|
||
| window.document.body.addEventListener('click', onInteractionOutside, false); |
There was a problem hiding this comment.
please use getRootDOMElement
| scrollContainer.addEventListener('scroll', calculateAndSetItemsListStyles, false); | ||
|
|
||
| return () => { | ||
| window.document.body.removeEventListener('click', onInteractionOutside); |
There was a problem hiding this comment.
please use getRootDOMElement
| import { getRootDOMElement } from './context.helper'; | ||
|
|
||
| const createDynamicRoot = ({ contextDOMElement = getRootDOMElement(), id } = {}) => { | ||
| if (id && contextDOMElement.querySelector(`#${id}`) !== null) { |
There was a problem hiding this comment.
nitpick: getElementById seems to be simpler
| if (id && contextDOMElement.querySelector(`#${id}`) !== null) { | |
| if (id && contextDOMElement.getElementById(id) !== null) { |
There was a problem hiding this comment.
getElementById exists only on document and here we have any possible element (currently - body), so I can't use it.
But at the same time, instead of contextDOMElement I can use just window.document, id has to be unique anyway...
| const DraggableDialog = ({ children, referenceElement, positionOffset }) => { | ||
| const rootDOMElement = getRootDOMElement(); | ||
| const containerRef = useRef(); | ||
| const dragOffsetPosition = useRef({ x: 0, y: 0 }); |
There was a problem hiding this comment.
Was the react state too slow or were there other problems with it? 🤔
There was a problem hiding this comment.
Other problems - this variable is used in drag handlers - it's recalculated (and also used) on mousemove event, which keeps state from moment when drag started, so too old for its purpose.
| rootDOMElement.removeEventListener('mousemove', handleDragging); | ||
| rootDOMElement.removeEventListener('mouseup', stopDragging); | ||
| }; | ||
| }, [isDragging]); |
There was a problem hiding this comment.
| }, [isDragging]); | |
| }, [isDragging, handleDragging, stopDragging]); |
| if (isDragging) { | ||
| rootDOMElement.addEventListener('mousemove', handleDragging, false); | ||
| rootDOMElement.addEventListener('mouseup', stopDragging, false); | ||
| } |
There was a problem hiding this comment.
No need to return a callback which removes listeners if they have not been added.
| if (isDragging) { | |
| rootDOMElement.addEventListener('mousemove', handleDragging, false); | |
| rootDOMElement.addEventListener('mouseup', stopDragging, false); | |
| } | |
| if (!isDragging) { | |
| return; | |
| } | |
| rootDOMElement.addEventListener('mousemove', handleDragging, false); | |
| rootDOMElement.addEventListener('mouseup', stopDragging, false); |
| x, | ||
| y, | ||
| }); | ||
| }, [referenceElement]); |
There was a problem hiding this comment.
Missing hook's dependencies.
|
|
||
| setItemsListStyles({}); | ||
| }; | ||
| }, [onClose, scrollContainer]); |
|
|
||
| setItemsListStyles(itemsStyles); | ||
| }; | ||
| const renderSearch = () => { |
There was a problem hiding this comment.
Maybe we can split this component into smaller ones? WDYT?
GrabowskiM
force-pushed
the
ai-assistant
branch
from
829825d to
634d284
Compare
tomaszszopinski
force-pushed
the
ai-assistant
branch
from
634d284 to
88a12c8
Compare
tomaszszopinski
force-pushed
the
ai-assistant
branch
from
88a12c8 to
ea5dac7
Compare
|
Description:
This PR provides two reusable react components - draggable dialog (that will be used for AI assistant) and popup menu (based a little on dropdown component, using similar styles as popup menu from vanilla JS)
For QA:
Documentation: