Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various fixes and improvements to enable installation into Learning MFE #6

Merged
merged 6 commits into from
Aug 18, 2023

Conversation

MichaelRoytman
Copy link
Member

@MichaelRoytman MichaelRoytman commented Aug 16, 2023

This pull request represents a set of fixes and improvements necessary to install this library into the Learning MFE. Additional work on this library is necessary before it's possible to install it into the Learning MFE (e.g. styling fixes, component structure improvements, changes to the Learning MFE, etc.), but this pull request contains an initial set of fixes for review.

This pull request includes the following changes:

  • moving dependencies from dependencies to devDependencies and peerDependencies in package.json
  • fixing the way the learningAssistant reducer is exported from slice.js
  • fixing the way we call thunks by calling the useDispatch hook to get a reference to the dispatch function and then dispatching the thunks
  • fixing the way the messageList array is indexed it following the removal of the default message in feat: add chat components #4
  • updating the library documentation with instructions on how to install this library into the Learning MFE for development
  • adding a mechanism to clear the currentMessage when a learner submits a message

JIRA: MST-2034 (private)

@MichaelRoytman MichaelRoytman force-pushed the michaelroytman/MST-2034-improvements-and-testing branch 4 times, most recently from 7961d92 to 97bcdf1 Compare August 16, 2023 21:48
@MichaelRoytman MichaelRoytman changed the title [WIP] Various fixed and improvements to enable installation into Learning MFE Aug 16, 2023
Copy link
Member

@alangsto alangsto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these look good, thanks for catching them 😄

@@ -37,6 +37,34 @@ documented in the repository README.

.. _Learning MFE: https://github.com/openedx/frontend-app-learning

Development
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this section!

Certain npm modules need to be the only version installed in an application. If a dependency is listed as a dependency of frontend-lib-learning-assistant and as a dependency of the parent application - frontend-app-learning, in this case - then there will be two copies of that module installed in the parent application. npm modules like react and @edx/frontend-platform need to be the only copy of those modules installed.

In other cases, it is not necessary to do this to enable the application to function, but it does help reduce the bundle size.

This commit moves npm modules that can be expected to be installed in the parent application from the dependencies array to the devDependencies and peerDependencies array.
This commit fixes an error in the way we're exporting the generated slice reducer.
This commit fixes a bug where thunks were not being dispatched correctly. This is fixed by using the useDispatch hook to get a referenceto the dispatch function, which is then applied to the thunk.
When the messageList is empty, indexing into the array returns undefined. Reading a property of undefined causes an error, so we use optional chaining in this case.
@MichaelRoytman MichaelRoytman force-pushed the michaelroytman/MST-2034-improvements-and-testing branch from 97bcdf1 to 54a5d6c Compare August 17, 2023 19:00
@MichaelRoytman MichaelRoytman changed the title Various fixed and improvements to enable installation into Learning MFE Various fixes and improvements to enable installation into Learning MFE Aug 17, 2023
This commit clears the currentMessage when a learner submits their message to the chatbot.
@MichaelRoytman MichaelRoytman force-pushed the michaelroytman/MST-2034-improvements-and-testing branch from a4e4230 to ae66e90 Compare August 17, 2023 19:29
@MichaelRoytman MichaelRoytman merged commit 965ba9b into main Aug 18, 2023
3 checks passed
@MichaelRoytman MichaelRoytman deleted the michaelroytman/MST-2034-improvements-and-testing branch August 18, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants