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

fix: prevent content tools from displaying over chat sidebar #1179

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

alangsto
Copy link
Contributor

@alangsto alangsto commented Aug 31, 2023

MST-2015

This PR fixes a UI bug when both the learning assistant and content tools are enabled for a course.

When content tools are enabled, they display over the learning assistant toggle:

Screenshot 2023-08-31 at 8 44 37 AM

If the learning sidebar is open, the content tools are similarly displayed over the chat sidebar:

Screenshot 2023-08-31 at 11 36 17 AM

The content tools should be conditionally displayed, depending on whether or not the chat sidebar is open:

Screenshot 2023-08-31 at 11 38 53 AM

Screenshot 2023-08-31 at 11 39 12 AM

@alangsto
Copy link
Contributor Author

Note that edx/frontend-lib-learning-assistant#24 should be merged prior to this PR. The version of the library should be updated as part of this PR.

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (ee80b24) 87.88% compared to head (3e99844) 87.85%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1179      +/-   ##
==========================================
- Coverage   87.88%   87.85%   -0.04%     
==========================================
  Files         263      263              
  Lines        4475     4480       +5     
  Branches     1129     1132       +3     
==========================================
+ Hits         3933     3936       +3     
- Misses        528      530       +2     
  Partials       14       14              
Files Changed Coverage Δ
src/courseware/course/chat/Chat.jsx 100.00% <ø> (ø)
src/courseware/course/Course.jsx 89.18% <100.00%> (+0.30%) ⬆️
...c/courseware/course/content-tools/ContentTools.jsx 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alangsto alangsto marked this pull request as ready for review August 31, 2023 20:33
@alangsto alangsto force-pushed the alangsto/fix_la_with_content_tools branch from 5218089 to 8a9dfb7 Compare August 31, 2023 20:41

return (
<div className="content-tools">
{!sidebarIsOpen && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Does the parent div needs to be rendered? Or should we hide the entire component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could hide the entire component, let me update

@alangsto alangsto force-pushed the alangsto/fix_la_with_content_tools branch from 8a9dfb7 to 3e99844 Compare August 31, 2023 20:53
@alangsto alangsto merged commit 32bd319 into master Aug 31, 2023
@alangsto alangsto deleted the alangsto/fix_la_with_content_tools branch August 31, 2023 21:09
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