IBX-9170: AI Assistant #1385

GrabowskiM · 2024-10-30T14:15:12Z

🎫 Issue	IBX-9170

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:

src/bundle/Resources/public/scss/ui/modules/common/_popup.menu.scss

OstafinL · 2024-11-20T12:34:59Z

src/bundle/ui-dev/src/modules/common/popup-menu/popup.menu.js

+    onClose: () => {},
+    onItemClick: () => {},
+    positionOffset: () => ({ x: 0, y: 0 }),
+    scrollContainer: window.document.body,


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 ?

dew326 · 2024-11-21T07:53:46Z

src/bundle/Resources/public/scss/ui/modules/common/_popup.menu.scss

+    &__item-content {
+        position: relative;
+        display: flex;
+        align-items: center;


duplicated align-items

dew326 · 2024-11-21T07:54:46Z

src/bundle/Resources/public/scss/ui/modules/common/_popup.menu.scss

+        cursor: pointer;
+        padding: calculateRem(9px);
+        color: $ibexa-color-dark;
+        font-size: calculateRem(14px);


we have variable for font-size

dew326 · 2024-11-21T07:55:54Z

src/bundle/Resources/public/scss/ui/modules/common/_popup.menu.scss

+        &:disabled,
+        &--disabled {
+            pointer-events: none;
+            cursor: not-allowed;


pointer-events: none and cursor: not-allowed, is it working together?

dew326 · 2024-11-21T07:59:26Z

src/bundle/ui-dev/src/modules/common/draggable-dialog/draggable.dialog.js

+
+    useEffect(() => {
+        if (isDragging) {
+            rootDOMElement.addEventListener('mousemove', handleDragging);


Suggested change

rootDOMElement.addEventListener('mousemove', handleDragging);

rootDOMElement.addEventListener('mousemove', handleDragging, false);

just for consistency

src/bundle/Resources/public/js/scripts/helpers/react.helper.js

OstafinL · 2024-11-28T12:38:25Z

src/bundle/ui-dev/src/modules/common/popup-menu/popup.menu.js

+            return null;
+        }
+
+        const groupClassName = createCssClassNames({


I'm not sure if we should use createCssClassNames because this it's only one class and it's not conditional

OstafinL · 2024-11-28T12:43:30Z

src/bundle/Resources/public/js/scripts/helpers/react.helper.js

@@ -0,0 +1,24 @@
+const createDynamicRoot = (contextDOMElement = window.document.body, id) => {


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

OstafinL · 2024-11-28T12:44:44Z

src/bundle/ui-dev/src/modules/common/popup-menu/popup.menu.js

+            onClose();
+        };
+
+        window.document.body.addEventListener('click', onInteractionOutside, false);


please use getRootDOMElement

OstafinL · 2024-11-28T12:44:56Z

src/bundle/ui-dev/src/modules/common/popup-menu/popup.menu.js

+        scrollContainer.addEventListener('scroll', calculateAndSetItemsListStyles, false);
+
+        return () => {
+            window.document.body.removeEventListener('click', onInteractionOutside);


please use getRootDOMElement

tischsoic · 2024-12-04T11:48:45Z

src/bundle/Resources/public/js/scripts/helpers/react.helper.js

+import { getRootDOMElement } from './context.helper';
+
+const createDynamicRoot = ({ contextDOMElement = getRootDOMElement(), id } = {}) => {
+    if (id && contextDOMElement.querySelector(`#${id}`) !== null) {


nitpick: getElementById seems to be simpler

Suggested change

if (id && contextDOMElement.querySelector(`#${id}`) !== null) {

if (id && contextDOMElement.getElementById(id) !== null) {

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...

tischsoic · 2024-12-04T12:00:09Z

src/bundle/ui-dev/src/modules/common/draggable-dialog/draggable.dialog.js

+const DraggableDialog = ({ children, referenceElement, positionOffset }) => {
+    const rootDOMElement = getRootDOMElement();
+    const containerRef = useRef();
+    const dragOffsetPosition = useRef({ x: 0, y: 0 });


Was the react state too slow or were there other problems with it? 🤔

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.

tischsoic · 2024-12-04T12:01:06Z

src/bundle/ui-dev/src/modules/common/draggable-dialog/draggable.dialog.js

+            rootDOMElement.removeEventListener('mousemove', handleDragging);
+            rootDOMElement.removeEventListener('mouseup', stopDragging);
+        };
+    }, [isDragging]);


Suggested change

}, [isDragging]);

}, [isDragging, handleDragging, stopDragging]);

tischsoic · 2024-12-04T12:04:00Z

src/bundle/ui-dev/src/modules/common/draggable-dialog/draggable.dialog.js

+        if (isDragging) {
+            rootDOMElement.addEventListener('mousemove', handleDragging, false);
+            rootDOMElement.addEventListener('mouseup', stopDragging, false);
+        }


No need to return a callback which removes listeners if they have not been added.

Suggested change

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);

tischsoic · 2024-12-04T12:04:49Z

src/bundle/ui-dev/src/modules/common/draggable-dialog/draggable.dialog.js

+            x,
+            y,
+        });
+    }, [referenceElement]);


Missing hook's dependencies.

tischsoic · 2024-12-04T12:06:25Z

src/bundle/ui-dev/src/modules/common/popup-menu/popup.menu.js

+
+            setItemsListStyles({});
+        };
+    }, [onClose, scrollContainer]);


Missing dependencies.

tischsoic · 2024-12-04T12:07:01Z

src/bundle/ui-dev/src/modules/common/popup-menu/popup.menu.js

+
+        setItemsListStyles(itemsStyles);
+    };
+    const renderSearch = () => {


Maybe we can split this component into smaller ones? WDYT?

sonarqubecloud · 2024-12-11T06:42:13Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

tomaszszopinski

QA approved on Ibexa DXP 4.6 commerce.

GrabowskiM marked this pull request as draft October 30, 2024 14:15

github-actions bot assigned dew326 Oct 30, 2024

GrabowskiM force-pushed the ai-assistant branch from b11091b to b242305 Compare October 31, 2024 11:00

github-actions bot assigned ciastektk, mikadamczyk and ViniTou Oct 31, 2024

GrabowskiM changed the title ~~AI Assistant~~ IBX-9170: AI Assistant Nov 7, 2024

GrabowskiM force-pushed the ai-assistant branch from 42c2d1a to 60e8ef7 Compare November 7, 2024 16:36

GrabowskiM marked this pull request as ready for review November 7, 2024 16:37

GrabowskiM requested review from OstafinL, albozek, dew326 and tischsoic November 7, 2024 16:37

GrabowskiM force-pushed the ai-assistant branch from 60e8ef7 to 28e8501 Compare November 7, 2024 16:40

GrabowskiM marked this pull request as draft November 13, 2024 08:08

GrabowskiM force-pushed the ai-assistant branch from 25325d8 to 990cbc0 Compare November 19, 2024 16:05

GrabowskiM marked this pull request as ready for review November 19, 2024 16:05

albozek reviewed Nov 20, 2024

View reviewed changes

src/bundle/Resources/public/scss/ui/modules/common/_popup.menu.scss Show resolved Hide resolved

OstafinL reviewed Nov 20, 2024

View reviewed changes

dew326 requested changes Nov 21, 2024

View reviewed changes

albozek approved these changes Nov 26, 2024

View reviewed changes

GrabowskiM requested review from OstafinL and dew326 November 27, 2024 11:13

OstafinL requested changes Nov 28, 2024

View reviewed changes

GrabowskiM requested review from OstafinL and albozek November 28, 2024 13:52

OstafinL approved these changes Nov 28, 2024

View reviewed changes

albozek approved these changes Nov 28, 2024

View reviewed changes

tischsoic requested changes Dec 4, 2024

View reviewed changes

dew326 approved these changes Dec 5, 2024

View reviewed changes

GrabowskiM requested review from OstafinL, albozek, dew326 and tischsoic December 5, 2024 13:18

tischsoic approved these changes Dec 5, 2024

View reviewed changes

dew326 approved these changes Dec 5, 2024

View reviewed changes

albozek approved these changes Dec 5, 2024

View reviewed changes

OstafinL approved these changes Dec 6, 2024

View reviewed changes

GrabowskiM force-pushed the ai-assistant branch from 829825d to 634d284 Compare December 6, 2024 10:33

GrabowskiM added the Ready for QA label Dec 6, 2024

tomaszszopinski force-pushed the ai-assistant branch from 634d284 to 88a12c8 Compare December 9, 2024 11:10

GrabowskiM added 11 commits December 11, 2024 07:40

AI Assistant

fdcd5c4

footer

f3d997a

improve search; fix linter

6c08547

dialog hook

077ed1b

translations

2ac20f7

minor fixes

40b5c00

recalc actions width

eb53194

added BC fallback

02c768c

after cr

f8b30d4

added some root handling

2066b8d

added dependency arrays; break popupmenu component to smaller ones

ea5dac7

tomaszszopinski force-pushed the ai-assistant branch from 88a12c8 to ea5dac7 Compare December 11, 2024 06:40

tomaszszopinski approved these changes Dec 17, 2024

View reviewed changes

tomaszszopinski added QA approved and removed Ready for QA labels Dec 17, 2024

dew326 merged commit 5d3d38c into 4.6 Dec 17, 2024
29 checks passed

dew326 deleted the ai-assistant branch December 17, 2024 11:56

	rootDOMElement.addEventListener('mousemove', handleDragging);
	rootDOMElement.addEventListener('mousemove', handleDragging, false);

		@@ -0,0 +1,24 @@
		const createDynamicRoot = (contextDOMElement = window.document.body, id) => {

	if (id && contextDOMElement.querySelector(`#${id}`) !== null) {
	if (id && contextDOMElement.getElementById(id) !== null) {

	}, [isDragging]);
	}, [isDragging, handleDragging, stopDragging]);

IBX-9170: AI Assistant #1385

IBX-9170: AI Assistant #1385

Uh oh!

Conversation

GrabowskiM commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

For QA:

Documentation:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 11, 2024

Quality Gate passed

Uh oh!

tomaszszopinski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

GrabowskiM commented Oct 30, 2024 •

edited

Loading