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

Implement Section Configure Modal #15

Conversation

CefBoud
Copy link

@CefBoud CefBoud commented Nov 30, 2023

Intro

This MR implements the Section Configure modal.

Dependens on:

Private ref

BB-8218

Testing instructions:

Screenshots

Screenshot from 2023-11-30 19-06-48
Screenshot from 2023-11-30 19-06-53
Screenshot from 2023-11-30 19-07-29
Screenshot from 2023-11-30 19-07-00

@CefBoud CefBoud force-pushed the CefBoud/course-outline/section-configure branch 2 times, most recently from a996739 to 08844b4 Compare November 30, 2023 18:15
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (navin/course-outline/section@6567ed9). Click here to learn what that means.

❗ Current head 2a1e872 differs from pull request most recent head bccaf1f. Consider uploading reports for the commit bccaf1f to get more accurate results

Additional details and impacted files
@@                       Coverage Diff                       @@
##             navin/course-outline/section      #15   +/-   ##
===============================================================
  Coverage                                ?   88.53%           
===============================================================
  Files                                   ?      485           
  Lines                                   ?     7530           
  Branches                                ?     1600           
===============================================================
  Hits                                    ?     6667           
  Misses                                  ?      834           
  Partials                                ?       29           

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

<FormattedMessage {...messages.hideFromLearners} />
</Form.Checkbox>
<hr />
{showWarning && (
Copy link

@mavidser mavidser Dec 5, 2023

Choose a reason for hiding this comment

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

In the spec, it seems that rendering the alert not-live is unintended, and instead can be derived from the current state of the checkbox.

if checked: displays message (strange legacy behaviour: warning appears only on following renders only )

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for mentioning this and reminding me about it because I wanted to bring it up.

In the legacy behavior, the warning is displayed if the section is already hidden from learners. We warn the cms user that some previously hidden content will be displayed. This seems sensible to me.

Toggling the warning based on the state of the checkbox feels somewhat off. Especially if the initial state is the content being not hidden. I thought we could maybe raise this point. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@CefBoud You are right, it makes sense to only show the warning if the content was already hidden.
cc: @mavidser

Copy link

@mavidser mavidser left a comment

Choose a reason for hiding this comment

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

@CefBoud This looks good, save for a few nits. 👍

  • I tested the update behaviour and compared it with the legacy behaviour.
  • I read through the code

src/course-outline/card-header/CardHeader.jsx Outdated Show resolved Hide resolved
src/course-outline/data/thunk.js Outdated Show resolved Hide resolved
src/course-outline/configure-modal/ConfigureModal.test.jsx Outdated Show resolved Hide resolved
@navinkarkera navinkarkera force-pushed the navin/course-outline/section branch 3 times, most recently from 98fdf30 to 8734daa Compare December 5, 2023 11:12
@CefBoud CefBoud force-pushed the CefBoud/course-outline/section-configure branch 9 times, most recently from 9c972de to 06908a5 Compare December 6, 2023 15:35
@CefBoud CefBoud force-pushed the CefBoud/course-outline/section-configure branch from 06908a5 to 2a1e872 Compare December 7, 2023 09:08
Copy link
Member

@navinkarkera navinkarkera left a comment

Choose a reason for hiding this comment

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

@CefBoud 👍

  • I tested this: locally tested configure modal
  • I read through the code
  • I checked for accessibility issues: I am not able to focus via keyboard on save button from basic tab due to focus getting stuck inside Time picker. It seems to be an issue with DatePicker from react-datepicker which is not related to this PR.
  • Includes documentation

@navinkarkera navinkarkera force-pushed the CefBoud/course-outline/section-configure branch from 2a1e872 to bccaf1f Compare December 11, 2023 14:39
@navinkarkera navinkarkera merged commit 2bfa367 into navin/course-outline/section Dec 11, 2023
3 checks passed
@Agrendalath Agrendalath deleted the CefBoud/course-outline/section-configure branch December 19, 2023 20:53
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.

3 participants