volto icon indicating copy to clipboard operation
volto copied to clipboard

Enhanced navigation reducer in Volto

Open Hrittik20 opened this issue 1 year ago • 4 comments

Fixes: #5772

Hrittik20 avatar Mar 01 '24 10:03 Hrittik20

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...

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] avatar Mar 01 '24 10:03 netlify[bot]

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...

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] avatar Mar 01 '24 10:03 netlify[bot]

@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.

ichim-david avatar Mar 02 '24 07:03 ichim-david

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

Hrittik20 avatar Mar 02 '24 08:03 Hrittik20

@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?

ichim-david avatar Mar 08 '24 09:03 ichim-david

@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 avatar Mar 15 '24 09:03 Hrittik20

@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

ichim-david avatar Mar 17 '24 19:03 ichim-david