Mega menu fixes #910

wjhsf · 2023-01-19T16:36:39Z

Category stale time was not being properly checked.
Category fetch time was being set on page load, rather than when it was fetched. (If on page load is the desired behavior, then fetchTime is a misnomer...)
Loaded category data was Category | Error, but we just want Category.
Accidental error when root[itemsKey] is undefined.

Description

Types of Changes

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Documentation update
Breaking change (could cause existing functionality to not work as expected)
Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

(change1)

How to Test-Drive This PR

(step1)

Checklists

General

Changes are covered by test cases
CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

There are no changes to UI

or...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

Changes include a UI text update in the Retail React App (which requires translation)

packages/template-retail-react-app/app/contexts/index.js

wjhsf · 2023-01-19T16:38:30Z

packages/template-retail-react-app/app/contexts/index.js

-            window.localStorage.getItem(`${LOCAL_STORAGE_PREFIX}${id}-${locale}`)
-        )
-        if (storageItem || Date.now() < storageItem?.fetchTime + CAT_MENU_STALE_TIME) {
+        if (storageItem && Date.now() < storageItem.fetchTime + CAT_MENU_STALE_TIME) {


|| meant that we would always return the stored data, regardless of staleness.

Gotcha, my fault on missing this, I missed the || which was the relevant bit

wjhsf · 2023-01-19T16:38:56Z

packages/template-retail-react-app/app/contexts/index.js

        const res = await api.shopperProducts.getCategory({
            parameters: {
                id,
                levels
            }
        })
+        res.loaded = true


There used to be a lot of loaded props floating around. Is this one still necessary?

Yes, it's used in the list-menu/index.jsx component

packages/template-retail-react-app/app/contexts/index.js

wjhsf · 2023-01-19T16:49:28Z

packages/template-retail-react-app/app/contexts/index.js

+        const promises = root?.[itemsKey]?.map(async (cat) => await fetchCategoryNode(cat?.id, 2))
+        if (promises) {


Does the data from the API potentially lack [itemsKey]? What would be the consequence of having it default to an empty array if it is missing (way up in useState, not here)? We'd have to change relevant checks from root?.[itemsKey] to root[itemsKey].length, but how many of those are there? Are there other consequences that I'm not thinking of?

itemsKey is a default constant set in the app, and the API will always respond with categories (which is the hardcoded itemsKey

My preference is to keep optional chaining bits simply because they introduce fault tolerance that reduces the chance of customers seeing thrown errors. Perhaps the API later changes their response schema, it does not cause any harm to be "lucky" and prepared for such a thing even if it never happens

wjhsf · 2023-01-19T16:50:03Z

packages/template-retail-react-app/app/contexts/index.js

        const res = await api.shopperProducts.getCategory({
            parameters: {
                id,
                levels
            }
        })
+        res.loaded = true
+        window.localStorage.setItem(


Moved this to inside the fetch function, so that fetch time is set on fetch, rather than on every page load.

bfeister

This looks good aside from the useEffect(async () => bit which needs to be reverted. Thanks for the catch on STALE_TIME

useEffect expects the callback to return nothing or a function; returning a promise could break things.

bfeister · 2023-01-19T17:43:18Z

packages/template-retail-react-app/app/contexts/index.js

+        const promises = root?.[itemsKey]?.map(async (cat) => await fetchCategoryNode(cat?.id, 2))
+        if (promises) {
+            // TODO: Error handling when fetching data fails.
+            // Possibly switch to .allSettled to show partial data?


.allSettled could be good here. Good suggestion

* New year, new look! (#876) * New year, new look! * Use JS to compute current year. * Small bump to max deps allowed. Needed to so that non-code changes will pass CI. Hitting the limit can be addressed later. * Update from 2.5.0 (#881) * Starting release process for 2.5.0 * allow commerce react sdk to release * bump version to 2.5.0 (#879) * bump version to 2.5.0 * bump max packages * Begin development on 2.6.0 * Set commerce-sdk-react package to private (#882) * test-commerce: comment out not-implemented customer hooks (#877) * Change padding (#899) * [Snyk] Security upgrade eslint-import-resolver-webpack from 0.10.0 to 0.13.1 (#887) * fix: packages/internal-lib-build/package.json & packages/internal-lib-build/package-lock.json to reduce vulnerabilities The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-DEBUG-3227433 * upgrade eslint-webpack-plugin-import, regen lock file Co-authored-by: Alex Vuong <alex.vuong@salesforce.com> Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com> * Cache SLAS callback using request processor (#884) * cache callback in request processor * fix import path * cache callback for a year * use native URLSearchParams * revert use native URLSearchParams * Rotate fingerprint ssh for deploy commerce sdk doc (#905) * Feature/megamenu fixes (#875) * [W-11996527] WIP - commit megamenu spike results / findings to feature branch * resolve search error * move default variables to constants file * cleanup code * add aria-live to menu categories * lint * implement aria live and busy * fix failing tests * update mock categories data, implement aria busy for listmenu * update mock data and fix list-menu test * lint * resolve unmounted component error * temporary testing server errors fix * set spinner opacity to 0 and increase max package value * lint * remove mock data from app code, clean up * allow padding of categories with loaded: true to bypass initial useEffect API call * fix last failing test * Update list-menu/index.jsx Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com> * fix aria-busy implementation and cleanup * add more test cases - when there is no root categories * remove mit license comment * add depndency array to useeffect * globally mock getcategory api call * WIP * fix tests WIP * remove all instances of setupmockserver in test files * globally mock istokenvalid AND many other cleanup items * refactor tree walking code * add locale checks and time to localstorage * WIP removing tree walking logic * remove tree walking code, update setroot and extract stale time * add catch to promise all * lint * namespace constants, remove unnecessary try catch... * final cleanup * fix lint Co-authored-by: Brian Feister <bfeister@salesforce.com> Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com> Co-authored-by: Kevin He <kevin.he@salesforce.com> * Mega menu fixes (#910) * Remove unnecessary catch clause. * Update copyright. * Don't return error when category is expected. * Fix bug preventing cache invalidation. * Set fetchTime when data is fetched, rather than every page load. * Update comment. * Convert promise chain to async/await for readability. * Guard against missing category items. * Move comment inside code block. * Change useEffect back to promise chain, not async/await. useEffect expects the callback to return nothing or a function; returning a promise could break things. * GitHub Actions (#854) * Add test workflow * Use actions/checkout v3 * Don't use CI image * Add step install npm dependencies * Add setup action * Set DEVELOP and RELEASE env variables * Add required shell property * Update action.yml * Update action.yml * Adding test matrix * cleanup * Split setup windows and ubuntu * Update test.yml * Update test.yml * Add Lighthouse and smoketest scripts * Not using actions/cache for now * Add generated matrix * Add missing runs-on prop to generated matrix * Add Setup Node step to generated matrix * Add Setup machine to generated matrix * Add cron schedule * Add timeout-minutes 5 * Move timeout to generate project step * Add Run tests on generated projects * Use dynamic generated-project folder * Run tests on test-project and retail-react-app-demo * Add Push Bundle step * Skip flaky test * Disable fail-fast strategy * Use env variables * Re-arrange env * Add step before push bundle * cleanup * cleanup * Use temp test-env-3 * testing slack notifs * testing * add publish to npm step * fix indent * python-dev does not exist anymore * use python2 * increase max packages * test slack notifs * add snyk cli and datadog step * update mrt user credentials * testing slack with pwa kit channel * syntax * fix conditionals * test push bundle * add push bundle step for generated * syntax * fix syntax error * update slack payload * run steps in container * testing * refactor * syntax * sudo container error * testing * update * add pip * use different docker * no container * container * testing * add user to container * fix * syntax add shell * syntax errors * remove container, use act * syntax errors * add snyk audit and other syntax stuff * extract steps to own actions * add inputs for actions * add shell for steps in actions with run * project cannot be generated in action file * updated snyk token, uncommenting code * Fix typo. * Add missing appkey property. * Use snake_case names for legibility. * Restore missing clean check * Fix skipped conversion to snake_case. * Trim trailing whitespace. * Extract conditionals to vars and clean up vars. * Change env IS_TESTABLE_TEMPLATE to more clear IS_TEMPLATE_FROM_RETAIL_REACT_APP * Fix YAML breaking conditional. * Try explicitly checking value. * Try explicitly checking true/false string values. * Try string comparisons. * Fix bad YAML. * Replace " with ' * Get ready for the prime time! * Fail fast! * Update TODOs. * Clean up npm version management. * Add TODO to merge workflows. * Update step names. * End files with newline. * Run on pull_request to support forked repos. * Only run on push for develop/release. We can assume all other branches will eventually have a PR. * Only push to MRT when actually desired. * Get that JavaScript nonsense outta here! * Check DEVELOP in step conditional, not in action execution. * Add some TODOs. * Too many newlines! Co-authored-by: yunakim714 <yunakim@salesforce.com> Co-authored-by: yunakim714 <84923642+yunakim714@users.noreply.github.com> Co-authored-by: Will Harney <wharney@salesforce.com> Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com> * GitHub Actions fixes (#915) * Update is not fork check * Add single quote to cron expression * Add cron docs * Remove Circle CI config, fix IS_NOT_FORK (#921) * Fix develop branch name (#923) * Fix wrong proptypes (#924) * fix wrong proptypes * Update packages/template-retail-react-app/app/components/recommended-products/index.jsx Co-authored-by: Vincent Marta <vmarta@salesforce.com> Co-authored-by: Brian Feister <47546998+bfeister@users.noreply.github.com> Co-authored-by: Vincent Marta <vmarta@salesforce.com> * Update createOrder to send SLAS USID (#920) * Update createOrder to send SLAS USID * Modify some tests to include setting the header Co-authored-by: echessman <37908171+echessman@users.noreply.github.com> * Upgrade prettier to v2 (#926) * Upgrade to prettier v2 for modern TypeScript support. * Change trailing comma to prettier v1 default. Minimizes changes during v2 upgrade. * Apply prettier v2 formatting changes. Yaaay touching all of the files! * Set end of line to LF. * Remove unnecessary map statement (#929) * fix: packages/pwa-kit-runtime/package.json & packages/pwa-kit-runtime/package-lock.json to reduce vulnerabilities (#935) The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-UAPARSERJS-3244450 Co-authored-by: snyk-bot <snyk-bot@snyk.io> * Update `develop` with v2.6.0 (#939) * Starting release process for 2.6.0 * 🚢 Release v2.6.0 (#937) * Update changelog files * Set `commerce-sdk-react-preview` as public * Version 2.6.0 * Begin development on 2.7.0 * Clean changelog files * Allow support for multiple sites concurrently w/ `commerce-sdk-react` (#911) * Namespace storage keys using the current siteId. Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com> * @W-11920099 Re-write 'npm push' in Typescript, warn if Node deprecated (#763) * Rewrite ancient scripts in Typescript. * Show warnings on push when using a deprecated Node version. * Automatically select a `.mobify` credentials file based on `--cloud-origin`. * Fix broken CI – confusing path to package.json in the 'dist' directory! (#946) * Reliably look up project and pwa-kit-dev package.json in scripts * fix: handle special characters in `boldString` util (#942) + Add a new util for escaping special regex characters + Apply new util to `boldString` + Add and update util tests Co-authored-by: Brian Feister <47546998+bfeister@users.noreply.github.com> * Replace isomorphic jest mocks with msw handlers (#944) * replace most manual mocks * more replacements * more replacements in addresses test * lint * resolve flaky cart test * remove some jest mocks * replace all isomorphic mocks * cleanup * fix auth modal mocks * remove timers from auth hooks test, fix password test * remove timer from create account test * add timeout * cleanup --------- Co-authored-by: Brian Feister <bfeister@salesforce.com> * Remove the PersistentCache functionality (#949) * Initial pass at PersistentCache * Drop test coverage * Change test * Update customer baskets cache when there is a basket mutation (#945) * update customer baskets cache on basket mutations * Fix layout shift for mega menu (#952) * remove custom styling * remove theme in theme file * spinners in drawer menu should be visible * Serialize category data once only (#953) * serialize data only once * remove console log --------- Co-authored-by: Brian Feister <47546998+bfeister@users.noreply.github.com> * chore: update pwa-kit-dev eslint config (#950) + Bump `eslint-plugin-react` to latest version + Auto-detect React version Co-authored-by: Adam Raya <adamraya@users.noreply.github.com> * resolving testing/merge errors * Add Shopper Experience hooks (#958) * Initial commit * Update license * Update test project to use bjnl_dev * Fix usePage hook and Refactor test page * Add usePages hook * Add usePages pdp and plp test cases * Clean up * Update Changelog & Restore `zzrf-001` config --------- Co-authored-by: Ben Chypak <bchypak@salesforce.com> * add changelog * Apply changes from commerce react sdk in feature branch (#964) * apply change from commerce react sdk in feature branch * Remove wrong changlog * keep the same as develop * linting * fix formatMessage * bump sizes temporarily * fix product list tests * fix header tests * fix add to cart modal * skip tests and low test coverate temporarily * linting --------- Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com> Co-authored-by: Alex Vuong <52219283+alexvuong@users.noreply.github.com> Co-authored-by: vcua-mobify <47404250+vcua-mobify@users.noreply.github.com> Co-authored-by: Vincent Marta <vmarta@salesforce.com> Co-authored-by: Pavel <65617653+GoodNightBuddy@users.noreply.github.com> Co-authored-by: Snyk bot <snyk-bot@snyk.io> Co-authored-by: Alex Vuong <alex.vuong@salesforce.com> Co-authored-by: yunakim714 <84923642+yunakim714@users.noreply.github.com> Co-authored-by: Brian Feister <bfeister@salesforce.com> Co-authored-by: Adam Raya <adamraya@users.noreply.github.com> Co-authored-by: yunakim714 <yunakim@salesforce.com> Co-authored-by: Will Harney <wharney@salesforce.com> Co-authored-by: Brian Feister <47546998+bfeister@users.noreply.github.com> Co-authored-by: echessman <37908171+echessman@users.noreply.github.com> Co-authored-by: CC ProdSec <65211003+cc-prodsec@users.noreply.github.com> Co-authored-by: Ben Chypak <bchypak@mobify.com> Co-authored-by: Oliver Brook <o.brook@salesforce.com> Co-authored-by: Brad Adams <hi@breadadams.com> Co-authored-by: Kieran Haberstock <80915722+kieran-sf@users.noreply.github.com> Co-authored-by: Ben Chypak <bchypak@salesforce.com>

wjhsf added 7 commits January 19, 2023 10:45

Remove unnecessary catch clause.

385b75a

Update copyright.

7d9624b

Don't return error when category is expected.

8f65c12

Fix bug preventing cache invalidation.

85f17ad

Set fetchTime when data is fetched, rather than every page load.

cc12695

Update comment.

1d02fc7

Convert promise chain to async/await for readability.

e837de6

wjhsf requested a review from a team as a code owner January 19, 2023 16:36

wjhsf added 2 commits January 19, 2023 11:40

Guard against missing category items.

5021816

Move comment inside code block.

f78ba7b

wjhsf commented Jan 19, 2023

View reviewed changes

bfeister requested changes Jan 19, 2023

View reviewed changes

Change useEffect back to promise chain, not async/await.

cde7a5e

useEffect expects the callback to return nothing or a function; returning a promise could break things.

wjhsf requested a review from bfeister January 19, 2023 17:23

bfeister reviewed Jan 19, 2023

View reviewed changes

bfeister approved these changes Jan 19, 2023

View reviewed changes

bfeister merged commit 4f545d9 into develop Jan 19, 2023

wjhsf deleted the fix/megamenu branch March 17, 2023 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mega menu fixes #910

Mega menu fixes #910

wjhsf commented Jan 19, 2023 •

edited

Loading

wjhsf Jan 19, 2023

bfeister Jan 19, 2023

wjhsf Jan 19, 2023

bfeister Jan 19, 2023

wjhsf Jan 19, 2023

bfeister Jan 19, 2023

bfeister Jan 19, 2023

wjhsf Jan 19, 2023

bfeister left a comment

bfeister Jan 19, 2023

		const promises = root?.[itemsKey]?.map(async (cat) => await fetchCategoryNode(cat?.id, 2))
		if (promises) {

Mega menu fixes #910

Mega menu fixes #910

Conversation

wjhsf commented Jan 19, 2023 • edited Loading

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

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

bfeister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjhsf commented Jan 19, 2023 •

edited

Loading