fix: modal popup should be grouped with toggle button #40
Conversation
alangsto
force-pushed
the
alangsto/fix_modal_css
branch
from
511dcb5
to
74139b8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
=======================================
Coverage 82.35% 82.35%
=======================================
Files 13 13
Lines 204 204
Branches 27 27
=======================================
Hits 168 168
Misses 34 34
Partials 2 2 ☔ View full report in Codecov by Sentry. |
| @@ -72,47 +72,48 @@ const ToggleXpert = ({ | |||
| && (localStorage.getItem('completedLearningAssistantTour') || !isModalOpen) | |||
| ); | |||
|
|
|||
| const chatMargin = contentToolsEnabled ? 'mb-5' : 'mb-3'; | |||
There was a problem hiding this comment.
Question: is the 'mb-5': 'mb-3' something that is hard-coded? Is this okay? Kind of a css newb so if I'm missing something obvious here lemme know haha. I like how you separated this as a const though--looks a lot cleaner! Nice!
There was a problem hiding this comment.
It's a bootstrap class (see https://getbootstrap.com/docs/4.0/utilities/spacing/). It's just to accomadate for different margins depending on whether or not the content tools are enabled (and therefore displayed at the bottom of the page).
Before I had additional margin space added, but just as a class. I figured bootstrap would be better!
There was a problem hiding this comment.
Ooooh, cool! Nice! And thanks for clarifying.
Because the popup modal did not share the same parent div as our toggle component, the position of the modal popup and the toggle would differ as the courseware page loaded.
As part of this PR I also fixed some margins for both the toggle and the popup to ensure that they do not overlay over the course tools (i.e. calculator and notes).
Before:
After:
