-
Notifications
You must be signed in to change notification settings - Fork 4
HNT-885-custom-section-functionality #1254
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
Conversation
b3d1cad to
fc3c5ec
Compare
| onSubmit: (values, formikHelpers) => { | ||
| onSubmit(values, formikHelpers); | ||
| }, | ||
| validate: (values) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you doing validation here? can you add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the code here, and I can't immediately connect it to the boolean validation mentioned in the comment. It also seems unrelated to custom sections.
Claude Code says this validation duplicates Formik's built-in behavior and should be redundant. Could you check whether this validation can be removed?
src/curated-corpus/components/CustomSectionFormConnector/CustomSectionFormConnector.tsx
Outdated
Show resolved
Hide resolved
src/curated-corpus/components/CustomSectionFormConnector/CustomSectionFormConnector.tsx
Show resolved
Hide resolved
katerinachinnappan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! I've tested this out on dev & here is some feedback besides the other comments I left in this PR:
-
I am able to select a past
startDate/endDatewhen I create / edit a section. See this section I created here: https://curation-admin-tools.getpocket.dev/curated-corpus/custom-sections/8abdfba5-886e-4c49-8a53-14b73b843632/NEW_TAB_EN_US/
We should not allow this. We should only create aSCHEDULEDsection orLIVEsection, wherestartDate>=currentDate. Also you should clarify with the curators if they want to be able to create a section to start/end on the same day. The validation form currently prohibits this but our backend allows it(e.g. a section can start on the beginning of 10/01 & expire at the end of 10/01). -
When creating a new section item for a custom section, it should take me to the section details page, instead it takes me to the modal to add another section item. See here: https://github.com/user-attachments/assets/72bb3a56-f81c-409b-8c3d-c9891f55ce50
-
We missed this when reviewing 884 - but we need to have the disable/enable toggle switch for a custom section, like we do for ML sections:

-
Lets use our custom modal to alert when deleting a custom section, instead of using
window.confirm -
We should not use
windowmethod when removing a section item ("X" button), we should show the same exact modal we do when removing a section item from an ML section. Exactly the same modal with the same reasons. -
I don't see the ability to add a section item to a custom section from the corpus page. This is part of https://mozilla-hub.atlassian.net/browse/HNT-886
See the figma here: https://www.figma.com/design/lysw5IQf2yGZioWrJ8PTmo/Curation-Tool?node-id=1994-24954&m=dev -
When I click "edit" custom section, the "Title" field seems to be cutoff -- the form should look the same as when creating a custom section. See here:
src/curated-corpus/components/AddProspectFormConnector/AddProspectFormConnector.tsx
Outdated
Show resolved
Hide resolved
src/curated-corpus/components/CustomSectionDetails/CustomSectionDetails.tsx
Outdated
Show resolved
Hide resolved
src/curated-corpus/components/CustomSectionDetails/CustomSectionDetails.tsx
Outdated
Show resolved
Hide resolved
src/curated-corpus/components/CustomSectionDetails/CustomSectionDetails.tsx
Outdated
Show resolved
Hide resolved
src/curated-corpus/components/CustomSectionDetails/CustomSectionDetails.tsx
Outdated
Show resolved
Hide resolved
Implement edit custom section functionality: - Add EditCustomSectionModal with validation for required metadata - Call updateCustomSection mutation to persist changes - Navigate back to custom section details page after editing - Prevent selecting past dates for start and end dates - Allow start and end dates to be the same day - Ensure end date is not before start date Implement section item management: - Add ability to manually add items to sections from custom section details page - Add ability to add items to sections from corpus page via schedule form - Call createSectionItem mutation when adding items - Support adding both existing and new corpus items - Filter out expired sections from section dropdown - Add status labels (Scheduled/Live) to sections in dropdown Implement section deletion: - Add delete section functionality with confirmation modal - Call deleteCustomSection mutation for soft delete - Replace window.confirm with DeleteConfirmationModal component - Update tests to work with modal-based confirmation Additional improvements: - Replace hardcoded hex colors with curationPalette references - Add border, lightRed, and veryLightGrey to theme palette - Extract onSave handler into separate method for readability - Remove incorrect default values (topic: 'TECHNOLOGY') from form transformations - Use empty string defaults for controlled inputs to prevent React warnings - Fix prettier formatting throughout
3f50364 to
c0ca708
Compare
* chore: update build dependencies and tooling Upgraded webpack, dev server, and test infrastructure to address compatibility issues. Webpack configuration: - Updated webpack-dev-server to v5 with complete API migration - Migrated to new setupMiddlewares pattern from deprecated before/after hooks - Updated static file serving configuration (contentBase → static) - Fixed splitChunks configuration for webpack 5 compatibility - Removed deprecated options (futureEmitAssets, requireEnsure parser) - Updated PostCSS loader to use postcssOptions - Fixed TerserPlugin configuration for webpack 5 Node.js polyfills: - Added browser polyfills for process, Buffer, crypto, stream, and url - Configured webpack ProvidePlugin for global availability - Added TypeScript declarations for polyfilled globals Jest updates: - Migrated to Jest v30 with updated transformer API - Updated file and CSS transformers to return code objects - Fixed test matchers (toBeCalled → toHaveBeenCalled) - Added jest-environment-jsdom for Jest 28+ compatibility - Removed deprecated testRunner configuration Dev server: - Configured port 3000 as default - Updated client configuration for WebSocket connections - Migrated to new middleware registration pattern * fix: address PR feedback
mmiermans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got to src/curated-corpus/components/CustomSectionDetails/CustomSectionDetails.tsx. Submitting my review now in case you want to start on addressing some of these comments.
This PR is adding a lot of functionality! Great job pulling this together.
src/curated-corpus/components/CustomSectionForm/CustomSectionForm.validation.ts
Show resolved
Hide resolved
| if (itemToRemove) { | ||
| const input: RemoveSectionItemInput = { | ||
| externalId: itemToRemove.externalId, | ||
| deactivateReasons: [SectionItemRemovalReason.Other], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Katerina said:
we should show the same exact modal we do when removing a section item from an ML section. Exactly the same modal with the same reasons.
I was looking to verify this requirement in Jira, but I couldn't immediately find the Jira ticket for removing section items. If it's more than a couple lines, it would probably be best to address this in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you decide to tackle this in a future PR, please create a Jira ticket to track it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/curated-corpus/components/AddProspectFormConnector/AddProspectFormConnector.tsx
Show resolved
Hide resolved
src/curated-corpus/components/CustomSectionDetails/CustomSectionDetails.tsx
Outdated
Show resolved
Hide resolved
| onSubmit: (values, formikHelpers) => { | ||
| onSubmit(values, formikHelpers); | ||
| }, | ||
| validate: (values) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the code here, and I can't immediately connect it to the boolean validation mentioned in the comment. It also seems unrelated to custom sections.
Claude Code says this validation duplicates Formik's built-in behavior and should be redundant. Could you check whether this validation can be removed?
mmiermans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the remaining files; no further comments. 👍
mmiermans
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for applying all review feedback.
Deliver end-to-end custom section management for the curated corpus so curators can create, edit, and
delete sections, curate items (prospects, existing corpus entries, or brand-new URLs), and stage them
with date ranges, IAB categories, and hero content.
Key Flows Covered
Todos
Modifying Environment Variables
N/A – no environment variables were added or changed.
Deployment
No special deployment considerations required. Standard deployment process applies.
Tickets: