Enhanced navigation reducer in Volto

Hrittik20 commented 2 months ago

Fixes: #5772

netlify[bot] commented 2 months ago

Deploy Preview for plone-components ready!

Name	Link
Latest commit	19e49afcdde34f4966eefb83e7a11cecb8782596
Latest deploy log	https://app.netlify.com/sites/plone-components/deploys/660e1d565d7f6600072cbecb
Deploy Preview	https://deploy-preview-5817--plone-components.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 2 months ago

Deploy Preview for volto ready!

Name	Link
Latest commit	19e49afcdde34f4966eefb83e7a11cecb8782596
Latest deploy log	https://app.netlify.com/sites/volto/deploys/660e1d569c16210008c20b47
Deploy Preview	https://deploy-preview-5817--volto.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ichim-david commented 2 months ago

@Hrittik20 when you add a pull request you need to check the CI tests and find out what fails. unit-tests You can see that the unit tests fail for plone/volto. this means that you need to go inside your branch to packages/volto and run: pnpm test

jest testing will run and eventually you will get failures. hit f key and only the failed tests will run. From there you can make changes to the code or tests and when saving the failed tests will re-run. This way you will know if what you changed fixed the tests or not.

Hrittik20 commented 2 months ago

@ichim-david Yes, I'm working on it. I will soon make an updated PR

ichim-david commented 1 month ago

@ichim-david Yes, I'm working on it. I will soon make an updated PR

@Hrittik20 I think the navigation test can avoid being update as long as the id is filtered since we have the url field. @sneridagh what do you think, are we ok with having the id of the item added or should we remove it?

Hrittik20 commented 1 month ago

@ichim-david How about If I update navigation.js by adding a check within the reducer logic that compares the incoming navigation items with the existing ones based on their URLs?

function isSameItem(item1, item2) {
  return item1.url === item2.url;
}
export default function navigation(state = initialState, action = {}) {
  let hasExpander;
  switch (action.type) {
    case `${GET_NAVIGATION}_PENDING`:
      return {
        ...state,
        error: null,
        loaded: false,
        loading: true,
      };
    case `${GET_CONTENT}_SUCCESS`:
      hasExpander = hasApiExpander(
        'navigation',
        getBaseUrl(flattenToAppURL(action.result['@id'])),
      );
      if (hasExpander && !action.subrequest) {
        const newItems = getRecursiveItems(
          action.result['@components'].navigation.items,
        );
        const hasSameItem = newItems.some(incomingItem =>
          state.items.some(existingItem => isSameItem(incomingItem, existingItem))
        );
        if (!hasSameItem) {
          return {
            ...state,
            error: null,
            items: newItems,
            loaded: true,
            loading: false,
          };
        }
      }
      return state;

ichim-david commented 1 month ago

@ichim-david How about If I update navigation.js by adding a check within the reducer logic that compares the incoming navigation items with the existing ones based on their URLs?

function isSameItem(item1, item2) {
  return item1.url === item2.url;
}
export default function navigation(state = initialState, action = {}) {
  let hasExpander;
  switch (action.type) {
    case `${GET_NAVIGATION}_PENDING`:
      return {
        ...state,
        error: null,
        loaded: false,
        loading: true,
      };
    case `${GET_CONTENT}_SUCCESS`:
      hasExpander = hasApiExpander(
        'navigation',
        getBaseUrl(flattenToAppURL(action.result['@id'])),
      );
      if (hasExpander && !action.subrequest) {
        const newItems = getRecursiveItems(
          action.result['@components'].navigation.items,
        );
        const hasSameItem = newItems.some(incomingItem =>
          state.items.some(existingItem => isSameItem(incomingItem, existingItem))
        );
        if (!hasSameItem) {
          return {
            ...state,
            error: null,
            items: newItems,
            loaded: true,
            loading: false,
          };
        }
      }
      return state;

@Hrittik20 I want to hear what @davisagli or @sneridagh has to say about this change, we might be ok to have also the id present without having todo any filtering, or we can simply filter it when destructuring it here https://github.com/plone/volto/pull/5817/files#diff-c1e66aca1aa305458203941b7e7d598522bbecc56ead1841087e2393b93a57b6R37

plone / volto