feat: xblock status component and copy & paste unit option #807

navinkarkera · 2024-01-25T05:25:32Z

Description:

Adds status text under each item in the outline. Screenshot from legacy UI for reference:

The reference logic for displaying this text can be found in https://github.com/openedx/edx-platform/blob/master/cms/templates/js/course-outline.underscore.

Also combines open-craft#29

MFE Screenshot:

Test instructions:

Get devstack up and running using make {lms,cms}-up.
Start npm devserver in this repo using npm start.
Goto http://localhost:2001/course/course-v1:edX+DemoX+Demo_Course/
Different types of status text should be visible based on the item configuration.
You can enable proctoring exam via advanced settings page.

Depends on: openedx/edx-platform#34058

openedx-webhooks · 2024-01-25T05:25:37Z

Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2024-01-25T05:34:00Z

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (056a15b) 90.60% compared to head (a1d669a) 90.67%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/course-outline/data/thunk.js	79.31%	6 Missing ⚠️
src/course-outline/utils.jsx	0.00%	6 Missing ⚠️
src/course-outline/hooks.jsx	85.71%	1 Missing ⚠️
src/course-outline/paste-button/PasteButton.jsx	94.73%	1 Missing ⚠️
...se-outline/xblock-status/GradingTypeAndDueDate.jsx	97.05%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
+ Coverage   90.60%   90.67%   +0.06%     
==========================================
  Files         511      521      +10     
  Lines        8901     9113     +212     
  Branches     1918     1971      +53     
==========================================
+ Hits         8065     8263     +198     
- Misses        811      825      +14     
  Partials       25       25

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

xitij2000

This is looking good. Just a few comments. I haven't tested it yet, but though I'd give an early round of feedback

xitij2000 · 2024-01-25T07:45:15Z

src/course-outline/xblock-status/XBlockStatus.scss

+.grading-mismatch-alert {
+  background: #FFFADB;
+  font-size: 14px;
+  font-weight: 400;
+  color: #454545;
+}


This is equivalent to:
bg-warning-100 small font-weight-normal text-gray-700

Using bg-warning-100 h4 font-weight-normal text-gray-700 makes it almost same as current implementation

Current look:

Using paragon classes:

It looks good enough to me.

xitij2000 · 2024-01-25T09:04:44Z

src/course-outline/section-card/SectionCard.scss

I don't think you need most of these classes for highlights-badge, you can replace them with the following classes:
d-flex justify-content-center align-items-center font-weight-bold

This will leave just he height, width and border radius. I think the font size can be skipped, 1 rem is just resetting the size to the root size so it should be the default anyway.

Removed most of the custom styles.

I think the font size can be skipped, 1 rem is just resetting the size to the root size so it should be the default anyway

For some reason, the font becomes smaller when we remove it.

xitij2000 · 2024-01-25T09:08:12Z

src/course-outline/section-card/SectionCard.scss

      font-weight: 700;
    }
  }
+
+  .section-card__content {
+    margin-left: 1.7rem;


Is this very specific margin needed? If not, you can go for ml-4 for 1.5rem or ml-4.5 for 2rem.

Additionally, I think this style of class naming is used by Paragon and libraries but isn't the convention in the apps.

Is this very specific margin needed?

Yes, to align content with the heading.

Additionally, I think this style of class naming is used by Paragon and libraries
but isn't the convention in the apps.

These classes were implemented before we started working on it, so kept the same naming convention.

xitij2000 · 2024-01-25T09:24:22Z

src/course-outline/utils.jsx

+      borderLeft: '5px solid #000000',
+    };


This pattern here is not good for theming. What I'd recommend instead is to have this function return classes.

Create a new class called border-l-5 with the following:

.border-5 { boder-left-width: 5px; border-left-style: solid; }

This is common styling for all, and after that, you can have the following code to get the style:

const getItemStatusBorder = (status) => { const statusBorder = { [ITEM_BADGE_STATUS.live]: 'border-info-500', [ITEM_BADGE_STATUS.publishedNotLive]: 'border-success-500', [ITEM_BADGE_STATUS.gated]: 'border-black', [ITEM_BADGE_STATUS.staffOnly]: 'border-black', [ITEM_BADGE_STATUS.unpublishedChanges]: 'border-accent-b', [ITEM_BADGE_STATUS.draft]: 'border-accent-b', }; return statusBorder[status] ?? ''; };

Then the consumer can just apply border-l-5 along with the above class.

I agree but the consumer is a component called SortableItem from frontend-lib-content-components and it only accepts css style object.

xitij2000 · 2024-01-25T10:06:24Z

src/course-outline/xblock-status/XBlockStatus.jsx

+import messages from './messages';
+import { COURSE_BLOCK_NAMES } from '../constants';
+
+const XBlockStatus = ({


Can we split this component into multiple parts? Currently, it's too large.
Here is what I'd suggest breaking each function insider the component into its own react component. They can all be in the same file but it'll clean up things a bit.

Also, I think instead of a generic name like item perhaps it could be called blockData or something more descriptive.

Done. Let me know if it the file still seems big, we can probably split into multiple files.

xitij2000 · 2024-01-25T10:24:43Z

src/course-outline/xblock-status/XBlockStatus.test.jsx

I think there is too much reliance on query/find/getByTestId. Using a test-id should be the last resort, and it isn't needed in most cases. I'll give a few alternates.

openedx-webhooks · 2024-01-30T10:29:31Z

I've created issue TNL-11410 in the private 2U Jira.

navinkarkera · 2024-01-30T10:30:30Z

@KristinAoki This is ready for you. The CI is failing due to some issue with TaxonomyListPage which is not related to this PR.

Combined open-craft#29 which implements copy & paste units feature

xitij2000

👍

I tested this: tested on tutor master
I read through the code

KristinAoki · 2024-01-31T16:22:40Z

src/course-outline/paste-button/PasteButton.jsx

+  }
+
+  const renderBlockLink = (props) => (
+    <Popover


This element should be a <ModalPopup> because <Popover> should not contain interactive elements due to accessibility.

Used Popover just for the arrow icon but ModalPopup doesn't seem to work nicely with hover events. Removed Popover and added the arrow icon via css and made the Hyperlink popup using overlay component directly. The div is accessible similar to how it was in legacy.

Noticed this with the change. Either the z-index needs to be adjusted or we should just use `ModalPopup` even though it does not support a hover state. `ModalPopup` has support for the arrow icon.

Updated z-index.

src/course-outline/paste-button/PasteButton.jsx

KristinAoki · 2024-01-31T16:25:52Z

src/course-outline/section-card/SectionCard.jsx

+            <XBlockStatus
+              isSelfPaced={isSelfPaced}
+              isCustomRelativeDatesActive={isCustomRelativeDatesActive}
+              blockData={section}
+            />


Move this below the section highlights

It is above section highlights in legacy, do we want to change it here?

src/course-outline/xblock-status/XBlockStatus.jsx

feat: add custom relative dates flag to state refactor: add gated status type refactor: alert style feat: add status text to units test: add tests fix: lint issues refactor: break up xblock status component fix: selector for isCustomRelativeDatesActive fix: prereq default value

refactor: paste component fix: lint issues and delete unused hook test: add test fix: update api for npm broadcast channel

openedx-webhooks · 2024-02-05T19:01:41Z

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 25, 2024

xitij2000 reviewed Jan 25, 2024

View reviewed changes

navinkarkera force-pushed the navin/course-outline/xblock-status-text branch from 4e12e3e to 9269a61 Compare January 26, 2024 13:17

navinkarkera requested a review from xitij2000 January 26, 2024 13:23

navinkarkera force-pushed the navin/course-outline/xblock-status-text branch from 253bc91 to 734b58b Compare January 29, 2024 12:56

navinkarkera marked this pull request as ready for review January 30, 2024 10:29

navinkarkera requested a review from KristinAoki January 30, 2024 10:29

navinkarkera added the jira:2u We want an issue in the 2U Jira instance label Jan 30, 2024

xitij2000 approved these changes Jan 30, 2024

View reviewed changes

navinkarkera changed the title ~~feat: xblock status component~~ feat: xblock status component and copy & paste unit option Jan 31, 2024

navinkarkera force-pushed the navin/course-outline/xblock-status-text branch 2 times, most recently from 01b94bd to bd13693 Compare January 31, 2024 10:14

KristinAoki requested changes Jan 31, 2024

View reviewed changes

navinkarkera force-pushed the navin/course-outline/xblock-status-text branch 2 times, most recently from f15707d to c9fc9c4 Compare February 1, 2024 06:48

navinkarkera requested a review from KristinAoki February 1, 2024 06:48

navinkarkera force-pushed the navin/course-outline/xblock-status-text branch 5 times, most recently from 7646fc8 to 018dcef Compare February 2, 2024 09:54

navinkarkera added 2 commits February 5, 2024 21:29

feat: copy & paste units

a1d669a

refactor: paste component fix: lint issues and delete unused hook test: add test fix: update api for npm broadcast channel

navinkarkera force-pushed the navin/course-outline/xblock-status-text branch from 018dcef to a1d669a Compare February 5, 2024 15:59

KristinAoki approved these changes Feb 5, 2024

View reviewed changes

KristinAoki merged commit 815ddbe into openedx:master Feb 5, 2024
6 checks passed

bradenmacdonald deleted the navin/course-outline/xblock-status-text branch March 11, 2024 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: xblock status component and copy & paste unit option #807

feat: xblock status component and copy & paste unit option #807

navinkarkera commented Jan 25, 2024 •

edited

Loading

openedx-webhooks commented Jan 25, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading

xitij2000 left a comment

xitij2000 Jan 25, 2024

navinkarkera Jan 26, 2024

xitij2000 Jan 25, 2024

navinkarkera Jan 26, 2024 •

edited

Loading

xitij2000 Jan 25, 2024

navinkarkera Jan 26, 2024

xitij2000 Jan 25, 2024

navinkarkera Jan 26, 2024

xitij2000 Jan 25, 2024

xitij2000 Jan 25, 2024

navinkarkera Jan 26, 2024

xitij2000 Jan 25, 2024

openedx-webhooks commented Jan 30, 2024

navinkarkera commented Jan 30, 2024 •

edited

Loading

xitij2000 left a comment

KristinAoki Jan 31, 2024

navinkarkera Feb 1, 2024

KristinAoki Feb 1, 2024

navinkarkera Feb 2, 2024

KristinAoki Jan 31, 2024

navinkarkera Feb 1, 2024

KristinAoki Feb 1, 2024

navinkarkera Feb 2, 2024

openedx-webhooks commented Feb 5, 2024

feat: xblock status component and copy & paste unit option #807

feat: xblock status component and copy & paste unit option #807

Conversation

navinkarkera commented Jan 25, 2024 • edited Loading

openedx-webhooks commented Jan 25, 2024 • edited Loading

codecov bot commented Jan 25, 2024 • edited Loading

Codecov Report

xitij2000 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

navinkarkera Jan 26, 2024 • edited Loading

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

openedx-webhooks commented Jan 30, 2024

navinkarkera commented Jan 30, 2024 • edited Loading

xitij2000 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

openedx-webhooks commented Feb 5, 2024

navinkarkera commented Jan 25, 2024 •

edited

Loading

openedx-webhooks commented Jan 25, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading

navinkarkera Jan 26, 2024 •

edited

Loading

navinkarkera commented Jan 30, 2024 •

edited

Loading