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

feat: content highlights stepper updates, including refactor to use context selectors and connects to Algolia for course search results #895

Merged
merged 16 commits into from
Dec 1, 2022

Conversation

brobro10000
Copy link
Member

@brobro10000 brobro10000 commented Nov 15, 2022

v2

  • Refactors to use use-context-selector for content highlights, which significantly reduces the performance impacts on component re-rendering when using our architectural approach to using vanilla React context; acts as a polyfill since official support for context selectors is still a proposal).
  • Continues on with the title step validation logic. Adds a validation message to the Stepper step description.
  • Persists data between the Stepper steps.
  • Disables DataTable row selection when the cap is reached.
  • Connects the "Select content" and "Confirm and and publish" steps with Algolia.
    • Note: requires configuring Algolia API environment variables. I'm using the production Algolia index for now (since stage course data is janky) with a hardcoded (test) production enterprise customer UUID. Recommended to use .env.private to set these environment variables for local development.
  • Sends POST to /api/v/1/highlight-sets-admin/ on final CTA click from "Confirm and publish" step to create highlight, prepending it to any existing highlight sets in the EnterpriseAppContext.enterpriseCuration data without needing to refresh from API.
  • Depends on upstream Paragon changes:
  • Depends on the Algolia index having aggregation_key configured as an attribute for faceting (PR incoming).

v1

Added Context, and refactored test/code to pass with context values for title and stepper logic
Initial work on data persist, more work is needed on it at this point
Set up reducer for persisting stepper data
Added logic to title to reflect limit on length of highlight title
Added additional UI changes to match language of the figma
Added reducer for 'Last deleted item' to be used in toast on deletion

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

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Base: 83.46% // Head: 83.52% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (100069c) compared to base (d36bff4).
Patch coverage: 83.73% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #895      +/-   ##
==========================================
+ Coverage   83.46%   83.52%   +0.05%     
==========================================
  Files         386      391       +5     
  Lines        8581     8727     +146     
  Branches     1825     1850      +25     
==========================================
+ Hits         7162     7289     +127     
- Misses       1387     1408      +21     
+ Partials       32       30       -2     
Impacted Files Coverage Δ
...Highlights/ContentHighlightsCardItemsContainer.jsx 83.33% <ø> (ø)
...mponents/ContentHighlights/HighlightSetSection.jsx 100.00% <ø> (ø)
...ts/EnterpriseApp/data/enterpriseCurationReducer.js 100.00% <ø> (ø)
src/components/settings/data/hooks.js 91.89% <ø> (ø)
...ents/ContentHighlights/ContentHighlightSetCard.jsx 66.66% <25.00%> (-24.25%) ⬇️
.../HighlightStepper/SelectContentSelectionStatus.jsx 27.27% <27.27%> (ø)
...nents/ContentHighlights/ContentHighlightRoutes.jsx 70.00% <40.00%> (-30.00%) ⬇️
...mponents/ContentHighlights/SkeletonContentCard.jsx 50.00% <50.00%> (ø)
...HighlightStepper/SelectContentSearchPagination.jsx 60.00% <60.00%> (ø)
...Stepper/HighlightStepperSelectContentDataTable.jsx 83.33% <83.33%> (ø)
... and 17 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.

@adamstankiewicz adamstankiewicz changed the base branch from hu/ent-6497 to master November 15, 2022 17:38
@brobro10000 brobro10000 force-pushed the hu/ent-6501 branch 2 times, most recently from 092d636 to 2b29eb0 Compare November 16, 2022 18:15
@brobro10000 brobro10000 marked this pull request as ready for review November 16, 2022 19:48
@adamstankiewicz adamstankiewicz changed the title feat: persist data through stepper and update UI and set context feat: content highlights stepper updates, including refactor to use context selectors and connects to Algolia for course search results Nov 29, 2022
@@ -68,9 +67,11 @@
"redux-mock-store": "1.5.4",
"redux-thunk": "2.3.0",
"regenerator-runtime": "0.13.7",
"scheduler": "^0.23.0",
Copy link
Member

Choose a reason for hiding this comment

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

required peer dependency of use-context-selector below

package.json Outdated Show resolved Hide resolved

const ContentHighlightRoutes = ({ enterpriseSlug }) => {
const baseContentHighlightPath = `/${enterpriseSlug}/admin/${ROUTE_NAMES.contentHighlights}`;
return (
<>
<Route
path={baseContentHighlightPath}
component={ContentHighlightsDashboard}
render={(routeProps) => (
<BaseContentHighlightRoute {...routeProps}>
Copy link
Member

Choose a reason for hiding this comment

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

[note] We will tackle this test coverage in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete

return history.push(`/${enterpriseSlug}/admin/${ROUTE_NAMES.contentHighlights}/${highlightSetUUID}`);
// redirect to individual highlighted set based on uuid
history.push(`/${enterpriseSlug}/admin/${ROUTE_NAMES.contentHighlights}/${highlightSetUUID}`);
return;
Copy link
Member

Choose a reason for hiding this comment

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

[note] We will tackle this test coverage in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete

Comment on lines -32 to -34
<p>
Create up to 8 highlight collections for your learners.
</p>
Copy link
Member

Choose a reason for hiding this comment

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

D'oh, I think this got erroneously deleted somehow, referring to the Figma:

image

Let's defer on changing in this PR though, as we can treat that as a distinct chunk of work to ensure only 8 highlight sets can be created and add the messaging back in then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete

};

const PriceTableCell = ({ row }) => {
const contentPrice = row.original.firstEnrollablePaidSeatPrice;
Copy link
Member

Choose a reason for hiding this comment

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

[note] tests will be added for covered in a future PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete

const handleChange = (e) => {
if (e.target.value.length > 60) {
setIsInvalid(true);
setHighlightTitle({
Copy link
Member

Choose a reason for hiding this comment

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

[note] A test will be added in future PR for this coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete

indeterminate,
checked,
...toggleRowSelectedProps
} = row.getToggleRowSelectedProps();
Copy link
Member

Choose a reason for hiding this comment

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

Test coverage will be added in a separate PR here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete


const SelectContentSelectionStatus = ({ className }) => {
const { toggleAllRowsSelected } = useContext(DataTableContext);
const currentSelectedRowsCount = useContextSelector(
Copy link
Member

Choose a reason for hiding this comment

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

[note] Test coverage will be added in a future PR here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving open as a reminder to complete

@brobro10000 brobro10000 merged commit 5fa390e into master Dec 1, 2022
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