MWPW-192158: Shared item loading layer for bulk publish#751
MWPW-192158: Shared item loading layer for bulk publish#751Andrei4226 wants to merge 8 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #751 +/- ##
==========================================
+ Coverage 87.32% 87.48% +0.15%
==========================================
Files 209 214 +5
Lines 62510 63141 +631
==========================================
+ Hits 54587 55238 +651
+ Misses 7923 7903 -20
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…s from the common folder
| import { getService } from '../../utils.js'; | ||
|
|
||
| const OFFER_DATA_CONCURRENCY_LIMIT = 5; | ||
| const VARIATIONS_CONCURRENCY_LIMIT = 5; |
There was a problem hiding this comment.
item-loading.js exports processConcurrently and these same constants, but nothing in src/ imports from it yet — only its own test does. Is the plan to migrate translation-items-loader.js to use item-loading.js in a follow-up, or is item-loading.js intended for a different consumer?
There was a problem hiding this comment.
I think the plan is to migrate also translation-items-loader.js to use item-loading.js.
So it’s better to have it in this PR as well.
Now, translation-items-loader.js imports the shared helpers directly from item-loading.js.
| import { CARD_MODEL_PATH, COLLECTION_MODEL_PATH, TABLE_TYPE } from '../../constants.js'; | ||
| import { Fragment } from '../../aem/fragment.js'; | ||
| import { getFragmentName } from '../../translation/translation-utils.js'; | ||
| import { getService } from '../../utils.js'; |
There was a problem hiding this comment.
The name translation-items-loader.js is translation-specific, and this file still imports getFragmentName from ../../translation/translation-utils.js. Moving it to common/ without generalizing it is the worst of both worlds — it either belongs back in translation/, or it needs to be renamed and have the translation-specific dependency inverted (e.g. accept a getDisplayName callback, as item-loading.js already does).
There was a problem hiding this comment.
Yes, you've right. I think it's better to rename it and have the translation‑specific dependency inverted.
So the getFragmentName import is removed. All functions now accept getDisplayName as a callback parameter, following the same pattern as item-loading.js.
| async connectedCallback() { | ||
| super.connectedCallback(); | ||
| setItemsSelectionStore(Store.translationProjects); | ||
|
|
There was a problem hiding this comment.
setItemsSelectionStore is called in connectedCallback but never reset in disconnectedCallback. If a child component renders before this editor mounts (or after it disconnects), it will read from whatever store was last set. Worth either resetting on disconnect or documenting the assumed single-instance constraint.
There was a problem hiding this comment.
Very good point. Now, the previous store is saved on connect and restored on disconnect via #itemsSelectionStoreSnapshot.
| * @param {AbortSignal} signal - Optional abort signal for cancellation | ||
| * @param {Number} timeoutMs - Timeout in milliseconds (default: 10000) | ||
| * @param {Number} timeoutMs - Timeout in milliseconds | ||
| * @returns {Promise<Object|null>} Offer data or null if not found/failed |
There was a problem hiding this comment.
The (default: 10000) annotation was dropped from the JSDoc. The default is still present in the function signature, but the doc hint was useful for callers — worth keeping.
There was a problem hiding this comment.
Restored the (default: 10000) annotation.
3 participants
Resolves https://jira.corp.adobe.com/browse/MWPW-192158
QA Checklist: https://wiki.corp.adobe.com/display/adobedotcom/M@S+Engineering+QA+Use+Cases
Extracts the shared item selection UI and data-loading logic from the Translation area into common/, making it reusable across features (Bulk Publish, Translations etc.). Decouples all UI components from Store.translationProjects via a new getItemsSelectionStore() abstraction.
Adds portable data-loading utils, shared rendering helpers and full unit test coverage.
Please review all functionalities from the "Select Items" section in the Translation area.
Please do the steps below before submitting your PR for a code review or QA
🧪 Nala E2E Tests
Nala tests run automatically when you open this PR.
To run Nala tests again:
run nalalabel to this PR (in the right sidebar)To stop automatic Nala tests:
run nalalabelTest URLs: