feat: Add highlight content card to stepper and individual sets #912

brobro10000 · 2022-12-05T16:48:12Z

Added initial cards for user selection in the highlight stepper content review section.
Added Price at the footer of the cards.
Added API integration with content highlight set items, displaying content highlight set items from highlight set.
Added Trashcan to remove items on the final step of the stepper
Added hyperlink on titles of cards
Added parameters/logics to take into account multiple content types for highlight cards

Added Relevant testing

Updating test reflecting changes and from reference PR #895

Highlight Set items

Step 2 of Highlight Stepper

Step 3 of Highlight Stepper

For all changes

Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

Ensure to attach screenshots
Ensure to have UX team confirm screenshots

src/components/ContentHighlights/ContentHighlightSet.jsx

src/components/ContentHighlights/data/hooks.js

src/components/ContentHighlights/ContentHighlightCardItem.jsx

adamstankiewicz · 2022-12-07T15:57:21Z

src/components/ContentHighlights/HighlightStepper/ContentHighlightStepper.jsx

@@ -69,7 +69,7 @@ const ContentHighlightStepper = ({ enterpriseId }) => {
      const newHighlightSet = {
        title: highlightTitle,
        isPublished: true,
-        // TODO: pass along the selected content keys!
+        content_key: Object.keys(currentSelectedRowIds).map(key => key.split(':')[1]),


Hmm, while this technically should work, it may not be optimal. That is, you're parsing the content_key from the aggregation_key field for the course. While the latter part of the aggregation_key is the format of content_key that enterprise-catalog expects here, it is a bit risky to assume that the content_key can be derived from the aggregation_key in this way, if that makes sense.

For example, content_key for courses in enterprise-catalog is derived from the key field, whereas content_key for programs/pathways content in enterprise-catalog is derived from the uuid field.

I think what you have now should be fine for now because your assumption is currently true, but we may want to add a TODO comment here to suggest finding a solution of extracting the appropriate content_key value from each selected highlighted content item based on its content type (i.e., key for courses, and uuid for programs/pathways).

We might try rubber ducking later on (outside of this PR) on finding a more optimal solution here.

Leaving Open as reference in the future

src/components/ContentHighlights/HighlightStepper/ContentHighlightStepper.jsx

src/components/ContentHighlights/HighlightStepper/ContentSearchResultCard.jsx

src/components/ContentHighlights/HighlightStepper/HighlightStepperConfirmContent.jsx

…-admin-portal into hu/ent-6562

…n-portal into hu/ent-6562

…-admin-portal into hu/ent-6562

codecov · 2022-12-13T21:23:05Z

Codecov Report

Base: 83.65% // Head: 83.73% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (d1d51b0) compared to base (ee876b0).
Patch coverage: 86.17% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
+ Coverage   83.65%   83.73%   +0.07%     
==========================================
  Files         391      393       +2     
  Lines        8768     8833      +65     
  Branches     1846     1855       +9     
==========================================
+ Hits         7335     7396      +61     
- Misses       1403     1407       +4     
  Partials       30       30

Impacted Files	Coverage Δ
...ontentHighlights/ContentHighlightCardContainer.jsx	`100.00% <ø> (ø)`
...nts/ContentHighlights/ContentHighlightsContext.jsx	`100.00% <ø> (ø)`
...s/ContentHighlights/ContentHighlightsDashboard.jsx	`93.33% <ø> (ø)`
...mponents/ContentHighlights/HighlightSetSection.jsx	`100.00% <ø> (ø)`
...ghtStepper/HighlightStepperSelectContentSearch.jsx	`83.33% <ø> (ø)`
src/data/services/EnterpriseCatalogApiService.js	`38.46% <0.00%> (-3.21%)`	⬇️
...mponents/ContentHighlights/ContentHighlightSet.jsx	`25.00% <25.00%> (-25.00%)`	⬇️
...ights/HighlightStepper/ContentHighlightStepper.jsx	`87.80% <50.00%> (-1.94%)`	⬇️
src/components/ContentHighlights/data/hooks.js	`84.44% <71.42%> (-11.39%)`	⬇️
src/components/ContentHighlights/data/utils.js	`92.30% <92.30%> (ø)`
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/components/ContentHighlights/ContentHighlightCardItem.jsx

src/components/ContentHighlights/ContentHighlightsCardItemsContainer.jsx

adamstankiewicz · 2022-12-16T15:54:29Z

src/components/ContentHighlights/ContentHighlightCardItem.jsx

+      <Hyperlink destination={hyperlink} target="_blank">
+        <Truncate lines={3} title={title}>{title}</Truncate>
+      </Hyperlink>


Hm, not sure a way around this, but it looks like the external link icon wraps onto a 4th line, since the text truncation doesn't consider the Hyperlink icon. And it looks like we can't have Truncate wrap Hyperlink instead.

I think for the purpose of this PR we should leave it as you have it, but let's confirm with Alena if this is OK. I personally feel having the external link icon sometimes wrap to a 4th line doesn't look all that clean and would possibly prefer a different approach to indicating opening in a new tab, e.g.:

Of course, let's defer to the decision is by Alena.

Leaving open for future reference

src/components/ContentHighlights/ContentHighlightSet.jsx

src/components/ContentHighlights/CurrentContentHighlightItemsHeader.jsx

src/components/ContentHighlights/data/constants.js

adamstankiewicz · 2022-12-16T16:29:31Z

src/components/ContentHighlights/data/constants.js

@@ -306,3 +330,68 @@ export const TEST_COURSE_HIGHLIGHTS_DATA = [
    ],
  },
 ];
+
+export const testCourseData = [


[clarification] Is this only used by tests?

Yep, it reflects sample data coming from the algolia's search clients hits that have been destructured to Content cards metadata

adamstankiewicz · 2022-12-16T16:29:43Z

src/components/ContentHighlights/data/constants.js

+  },
+];
+
+export const testCourseAggregation = {


[clarification] Is this only used by tests?

Yep, this represents the selectedRowIds between state changes of the stepper.

nit: Got it. I wonder if it'd be worth moving these 2 variables intended only for tests into a different file to not confuse with application constants (e.g., data/tests/constants.js)?

src/components/ContentHighlights/HighlightStepper/ContentConfirmContentCard.jsx

src/components/ContentHighlights/ContentHighlightCardItem.jsx

adamstankiewicz · 2022-12-19T15:52:30Z

src/components/ContentHighlights/ContentHighlightCardItem.jsx

+  }
+  const cardInfo = {
+    cardImgSrc: cardImageUrl,
+    cardLogoSrc: partners?.length === 1 ? partners[0].logoImageUrl : undefined,


nit: partners is listed as a required prop. if that's the case, we should be able to assume partners.length would work without the ?.. If that breaks tests, the test itself may need to ensure the partners is passed with an empty array vs. undefined.

adamstankiewicz · 2022-12-19T16:07:08Z

src/components/ContentHighlights/ContentHighlightCardItem.jsx

+  }
+  const cardInfo = {
+    cardImgSrc: cardImageUrl,
+    cardLogoSrc: partners?.length === 1 ? partners[0].logoImageUrl : undefined,


nit: partners is listed as a required prop. if that's the case, we should be able to assume partners.length would work without the ?.. If that breaks tests, the test itself may need to ensure the partners is passed with an empty array vs. undefined.

src/components/ContentHighlights/SkeletonContentCardContainer.jsx

adamstankiewicz · 2022-12-19T16:15:58Z

src/components/ContentHighlights/data/constants.js

+  },
+];
+
+export const testCourseAggregation = {


nit: Got it. I wonder if it'd be worth moving these 2 variables intended only for tests into a different file to not confuse with application constants (e.g., data/tests/constants.js)?

src/components/ContentHighlights/data/hooks.js

src/components/ContentHighlights/data/utils.js

adamstankiewicz · 2022-12-19T22:08:20Z

src/components/ContentHighlights/SkeletonContentCardContainer.jsx

+import SkeletonContentCard from './SkeletonContentCard';
+import { HIGHLIGHTS_CARD_GRID_COLUMN_SIZES } from './data/constants';
+
+const SkeletonContentCardContainer = ({ length, columnSizes }) => (


nit: length -> totalItems

adamstankiewicz · 2022-12-19T22:14:59Z

src/components/ContentHighlights/data/hooks.js

+  const deleteSelectedRowId = useCallback((rowId) => {
+    const currentRowIds = { ...currentSelectedRowState };
+    delete currentRowIds[rowId];
+    setState(s => ({


I believe we can nix the currentSelectedRowState state variable in favor of something like below:

const deleteSelectedRowId = useCallback((rowId) => { const currentRowIds = { ...currentSelectedRowState }; setState((s) => { const currentRowIds = { ...s }; delete currentRowIds[rowId]; return { ...s, stepperModal: { ...s.stepperModal, currentSelectedRowIds: currentRowIds, }, }; }); });

feat: Add highlight content card to stepper and individual sets

ee6e285

brobro10000 marked this pull request as ready for review December 7, 2022 14:56

feat: Add highlight content card to stepper and individual sets

d1e2ec0

brobro10000 force-pushed the hu/ent-6562 branch from d7efc93 to d1e2ec0 Compare December 7, 2022 15:00

adamstankiewicz reviewed Dec 7, 2022

View reviewed changes

brobro10000 added 6 commits December 9, 2022 09:09

feat: Added delete to individual cards

8c09af9

Merge branch 'master' into hu/ent-6562

5de4ae0

chore: Unit testing

f699452

Merge branch 'hu/ent-6562' of https://github.com/openedx/frontend-app…

cd6cdae

…-admin-portal into hu/ent-6562

Merge branch 'hu/ent-6562' of https://github.com/openedx/frontend-app…

97876cc

…-admin-portal into hu/ent-6562

Merge branch 'hu/ent-6562' of https://github.com/openedx/frontend-app…

db03859

…-admin-portal into hu/ent-6562

brobro10000 force-pushed the hu/ent-6562 branch from 94382b1 to db03859 Compare December 9, 2022 18:48

brobro10000 added 3 commits December 9, 2022 13:48

Merge branch 'master' of https://github.com/openedx/frontend-app-admi…

58e5861

…n-portal into hu/ent-6562

Merge branch 'master' of https://github.com/openedx/frontend-app-admi…

e7bafe7

…n-portal into hu/ent-6562

Merge branch 'hu/ent-6562' of https://github.com/openedx/frontend-app…

1e9f486

…-admin-portal into hu/ent-6562

brobro10000 added 2 commits December 13, 2022 17:43

feat: Add hyperlinks on card titles

edce596

chore: Update test for mapToStore enterpriseSlug

d77cd5e

adamstankiewicz reviewed Dec 16, 2022

View reviewed changes

brobro10000 force-pushed the hu/ent-6562 branch from 19caed8 to f2dc1e6 Compare December 16, 2022 20:03

feat: PR fixes

dc1d591

brobro10000 force-pushed the hu/ent-6562 branch from f2dc1e6 to dc1d591 Compare December 16, 2022 22:31

Merge branch 'master' into hu/ent-6562

60587a7

adamstankiewicz reviewed Dec 19, 2022

View reviewed changes

brobro10000 and others added 4 commits December 19, 2022 15:45

chore: tests

7bc6c4f

chore:test part 2

cd1782c

chore: update tests

722df03

chore: update tests... again

17b8baa

adamstankiewicz approved these changes Dec 19, 2022

View reviewed changes

chore: PR feedback

d1d51b0

brobro10000 merged commit 6d64163 into master Dec 19, 2022

brobro10000 deleted the hu/ent-6562 branch December 19, 2022 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add highlight content card to stepper and individual sets #912

feat: Add highlight content card to stepper and individual sets #912

brobro10000 commented Dec 5, 2022 •

edited

Loading

adamstankiewicz Dec 7, 2022

adamstankiewicz Dec 7, 2022

brobro10000 Dec 13, 2022

codecov bot commented Dec 13, 2022 •

edited

Loading

adamstankiewicz Dec 16, 2022

brobro10000 Dec 16, 2022

adamstankiewicz Dec 16, 2022

brobro10000 Dec 16, 2022

adamstankiewicz Dec 16, 2022

brobro10000 Dec 16, 2022

adamstankiewicz Dec 19, 2022

adamstankiewicz Dec 19, 2022

adamstankiewicz Dec 19, 2022

adamstankiewicz Dec 19, 2022

adamstankiewicz Dec 19, 2022

adamstankiewicz Dec 19, 2022

feat: Add highlight content card to stepper and individual sets #912

feat: Add highlight content card to stepper and individual sets #912

Conversation

brobro10000 commented Dec 5, 2022 • edited Loading

For all changes

Only if submitting a visual change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brobro10000 commented Dec 5, 2022 •

edited

Loading

codecov bot commented Dec 13, 2022 •

edited

Loading