Fix Xpert styling issues, fix lingering bugs, and make improvements. #8

MichaelRoytman · 2023-08-18T18:15:06Z

Description

This pull request adds proper styling to the Xpert chatbot to closely resemble the Hackathon UI/UIX and also introduces a few fixes to the Xpert chatbot.

These changes include:

Styling

converting Tailwind classes to Bootstrap classes, when equivalent Bootstrap classes exist
convert Tailwind classes to CSS, when equivalent Bootstrap classes do not exist
convert CSS to SCSS to improve readability
leverage flexbox to remove absolute positioning, where possible
show and hide the Sidebar by conditionally rendering the component instead of using opacity and positioning

Behavior

correctly dispatch the addChatMessage thunk action creator
move the API loading state to Redux
move API call from Xpert widget to Sidebar component
store serializable date value in Redux
fix propType for chatbotContainerRef
scroll down to bottom of the chat window after reopening the Xpert chatbot

JIRA: MST-2034 (private)

Demos

Before

MST-2034-Xpert-Before-Demo.mov

After

Please note that the Xpert responses are mocked out. The Xpert chatbot is not making actual API calls in this demo.

MST-2034-Xpert-Demo.mov

alangsto

Incredible work! I don't see anything that jumps out right away, but I'll take a more detailed look at the CSS too

alangsto · 2023-08-21T18:29:02Z

src/data/thunks.js

+
+    // Redux recommends only serializable values in the store, so we'll stringify the timestap to store in Redux.
+    // When we need to operate on the Date object, we'll deserialize the string.
+    const timestamp = new Date();


Nice update 👍

alangsto · 2023-08-21T18:30:08Z

src/components/Sidebar/index.jsx

@@ -62,6 +67,7 @@ const Sidebar = ({
    event.preventDefault();
    if (currentMessage) {
      dispatch(addChatMessage('user', currentMessage));
+      dispatch(getChatResponse(courseId));


I like that this is moved closer to the action that triggers it

MichaelRoytman · 2023-08-18T18:22:07Z

src/widgets/Xpert.jsx

  const [sidebarIsOpen, setSidebarIsOpen] = useState(false);
-  const dispatch = useDispatch();


I've moved the API call logic from the Xpert widget to the Sidebar component. Currently, the API call to the Xpert backend is occurring as as a side effect of changes to the messageList Redux state in this useEffect, which is the parent component of the Sidebar. This is a side-effect that is difficult to follow. After this change, the API call occurs directly as a consequence of the user's action - enter or button click to submit the message. This makes it easier to follow how an API call is made.

MichaelRoytman · 2023-08-18T18:22:38Z

src/widgets/Xpert.jsx

      <Sidebar
+        courseId={courseId}


The Sidebar component needs the courseId prop to be able to dispatch the getChatResponse action creator.

MichaelRoytman · 2023-08-18T18:25:22Z

src/data/thunks.js

@@ -31,11 +32,15 @@ export function addChatMessage(role, content) {
 export function getChatResponse(courseId) {
  return async (dispatch, getState) => {
    const { messageList } = getState().learningAssistant;
+
+    dispatch(setApiIsLoading(true));


I've moved the API loading state into Redux. Before, it was part of the Chatbox component state. We were deriving the loading state from state in the Redux store that was being modified by a few different action creators, which meant that the "Xpert is thinking..." message wasn't showing properly.

Moving this state into the Redux store fixes the display of the "Xpert is thinking..." message, makes it easier to define and set this value, and makes it available to other components in the tree that may need it later (e.g. disabling the button during loading).

MichaelRoytman · 2023-08-18T18:31:11Z

src/components/Sidebar/index.jsx

-    if (maxScrollTop > 0) {
-      const startTime = Date.now();
-      const endTime = startTime + duration;
+    if (messageContainer) {


Because we are now conditionally hiding and showing the Sidebar instead of relying on opacity and position attributes, chatboxContainerRef will be undefined when the Sidebar is unmounted, because Chatbox is a child of Sidebar. For this reason, we do a truthy check here first to make sure the Chatbox is mounted.

MichaelRoytman · 2023-08-21T18:04:46Z

src/components/ChatBox/index.jsx

-const ChatBox = ({ messageList, chatboxContainerRef }) => {
-  const [isResponseLoading, setResponseLoading] = useState(false);
-
-  useEffect(() => {


I've moved the API loading state into Redux. Before, it was part of the Chatbox component state. We were deriving the loading state from state in the Redux store that was being modified by a few different action creators, which meant that the "Xpert is thinking..." message wasn't showing properly.

Moving this state into the Redux store fixes the display of the "Xpert is thinking..." message, makes it easier to define and set this value, and makes it available to other components in the tree that may need it later (e.g. disabling the button during loading).

MichaelRoytman · 2023-08-21T18:05:22Z

src/components/ChatBox/index.jsx

  })).isRequired,
-  chatboxContainerRef: PropTypes.string.isRequired,
+  chatboxContainerRef: PropTypes.object.isRequired,


The chatboxContainerRef is an object, not a string, so this was failing.

MichaelRoytman · 2023-08-21T18:09:22Z

src/components/Sidebar/index.jsx

    }
-  }, [messageList]);
+  }, [messageList, isOpen]);


Consequently, when we close and reopen the Sidebar, the component is remounted, and we lose the scroll position. Adding isOpen to the dependency array causes the Sidebar to scroll to the proper position. The learner does see this after the Sidebar opens, which may not be ideal. Please see the demo video.

MichaelRoytman · 2023-08-21T18:10:53Z

src/components/Sidebar/index.jsx

@@ -62,6 +67,7 @@ const Sidebar = ({
    event.preventDefault();
    if (currentMessage) {
      dispatch(addChatMessage('user', currentMessage));
+      dispatch(getChatResponse(courseId));


I've moved the API call logic from the Xpert widget to the Sidebar component. Currently, the API call to the Xpert backend is occurring as as a side effect of changes to the messageList Redux state in this useEffect, which is the parent component of the Sidebar. This is a side-effect that is difficult to follow. After this change, the API call occurs directly as a consequence of the user's action - enter or button click to submit the message. This makes it easier to follow how an API call is made.

This commit includes a number of CSS fixes and changes. The fundamental issue is that the existing Xpert code used Tailwind classes, which we do not have access to in the learning MFE. Furthermore, the existing Xpert code did not make use of Bootstrap. This commit replaces Tailwind classes with Bootstrap classes, where equivalent Bootstrap classes exist. Tailwind classes that did not have an equivalent Bootstrap class are replaced by equivalent CSS. The CSS files were converted to SCSS files. Other changes include conditionally rendering the Sidebar component instead of relying on opacity and absolute positioning to hide and show the Sidebar, remove unncessary styling, and relying on flexbox for positioning where possible over absolute positioning.

This commit fixes a number of bugs in the Xpert. These fixes include changing the propType for the chatboxContainerRef from a string to an object, storing serializable values for dates in Redux, and correctly dispatching the addChatMessage thunk action creator.

This commit makes a number of improvements to the Xpert code. These improvements include moving the API call logic from Xpert widget to Sidebar component to make it easier to follow how an API call is made and to avoid side effects, moving the API loading state to the Redux store to have a cleaner data flow and to make the data available to the entire component tree, and correctly scrolling to the bottom of the ChatBox when the Sidebar is opened.

MichaelRoytman force-pushed the michaelroytman/MST-2034-styling-improvements branch from eea2d54 to d602f1a Compare August 21, 2023 18:00

MichaelRoytman changed the title ~~[WIP] Styling Improvements - NOT READY FOR REVIEW~~ Fix Xpert styling issues and lingering bugs. Aug 21, 2023

MichaelRoytman force-pushed the michaelroytman/MST-2034-styling-improvements branch 2 times, most recently from 657e9ca to 3f71d65 Compare August 21, 2023 18:12

alangsto approved these changes Aug 21, 2023

View reviewed changes

MichaelRoytman commented Aug 21, 2023

View reviewed changes

MichaelRoytman force-pushed the michaelroytman/MST-2034-styling-improvements branch from 3f71d65 to a40df14 Compare August 21, 2023 19:15

MichaelRoytman changed the title ~~Fix Xpert styling issues and lingering bugs.~~ Fix Xpert styling issues, fix lingering bugs, and make improvements. Aug 21, 2023

MichaelRoytman force-pushed the michaelroytman/MST-2034-styling-improvements branch from a40df14 to e7188d1 Compare August 21, 2023 19:59

MichaelRoytman force-pushed the michaelroytman/MST-2034-styling-improvements branch 2 times, most recently from 758a1df to 1237118 Compare August 21, 2023 20:04

MichaelRoytman added 2 commits August 21, 2023 16:06

fix: Xpert bugs

57987df

This commit fixes a number of bugs in the Xpert. These fixes include changing the propType for the chatboxContainerRef from a string to an object, storing serializable values for dates in Redux, and correctly dispatching the addChatMessage thunk action creator.

MichaelRoytman force-pushed the michaelroytman/MST-2034-styling-improvements branch from 1237118 to c219134 Compare August 21, 2023 20:06

MichaelRoytman merged commit c9b7780 into main Aug 21, 2023
3 checks passed

MichaelRoytman deleted the michaelroytman/MST-2034-styling-improvements branch August 21, 2023 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Xpert styling issues, fix lingering bugs, and make improvements. #8

Fix Xpert styling issues, fix lingering bugs, and make improvements. #8

MichaelRoytman commented Aug 18, 2023 •

edited

Loading

alangsto left a comment

alangsto Aug 21, 2023

alangsto Aug 21, 2023

MichaelRoytman Aug 18, 2023

MichaelRoytman Aug 18, 2023

MichaelRoytman Aug 18, 2023

MichaelRoytman Aug 18, 2023

MichaelRoytman Aug 21, 2023

MichaelRoytman Aug 21, 2023

MichaelRoytman Aug 21, 2023

MichaelRoytman Aug 21, 2023

		const [sidebarIsOpen, setSidebarIsOpen] = useState(false);
		const dispatch = useDispatch();

Fix Xpert styling issues, fix lingering bugs, and make improvements. #8

Fix Xpert styling issues, fix lingering bugs, and make improvements. #8

Conversation

MichaelRoytman commented Aug 18, 2023 • edited Loading

Description

Demos

alangsto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelRoytman commented Aug 18, 2023 •

edited

Loading