Skip to content
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

[FEAT] Allow for queries/filters on collections when generating the sitemap #109

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Kyle772
Copy link

@Kyle772 Kyle772 commented Mar 1, 2023

What does it do?

This implements conditions into the collections queries.

Below is a SS of the UI change.

image

Why is it needed?

The general idea here is to make the sitemaps more flexible. In my case, I wanted to filter out anything that was removed from the website when an entry was marked as deleted (but not unpublished so we can retain data where necessary; ie invoicing).

How to test it?

Nothing major was added so you should be able to run this as you normally would

Related issue(s)/PR(s)

Let us know if this is related to any issue/pull request

I think this only impacts the below issue but there may be some others.

#99

Copy link
Author

@Kyle772 Kyle772 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick note about i18n

server/services/core.js Outdated Show resolved Hide resolved
@cipick
Copy link

cipick commented Jul 18, 2023

this is amazing. just want to thank @Kyle772 for this feature, i had a hard time to extend the plugin to do the same thing <3

@boazpoolman
Copy link
Member

I figured this feature deserves it's own section.
So I've split up the modal in tabs, where the "Filters" tab contains this feature.

Scherm­afbeelding 2023-07-19 om 10 48 44

@boazpoolman
Copy link
Member

I've updated the settings schema a bit.

The PR was initially propsed with a settings object that might look like this:

{
  "contentTypes": {
    "api::page.page": {
      "languages": { "und": {
        "pattern": "/pages/[id]",
        "condition1": "slug",
        "condition1Operator": "$not",
        "condition1Value": "test"
      }
    }
  }
}

I've updated the PR so that the filters will be saved separately from the other pattern settings. Which could lead to a settings object that might look like this:

{
  "contentTypes": {
    "api::page.page": {
      "languages": { "und": { "pattern": "/pages/[id]" },
      "filters": {
        "0": {
          "field": "slug",
          "operator": "$not",
          "value": "test"
        }
      }
    }
  }
}

dispatch(onChangeContentTypes(uid, null, ['filters', conditionCount, 'operator'], ''));
dispatch(onChangeContentTypes(uid, null, ['filters', conditionCount, 'value'], ''));
setConditionCount(conditionCount - 1);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handleRemoveCondition function always removes the last filter. It would be neat if we can specify the filter we want to remove. Giving the end user more control on the filters interface.

};
return (
<div>
{conditionCount === 0 ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition can be optimized.

The only thing that should/shouldn't show for this ternary is the list of conditions. Though this ternary is used to render the entire component leaving duplicate code below (e.g. the add/remove buttons).

If we use the ternary just for the conditions list this would be resolved.

@boazpoolman
Copy link
Member

Hi @Kyle772

Have you been able to take a look at the feedback I've posted on the PR?
I'd love to get this feature merged.

@Kyle772
Copy link
Author

Kyle772 commented Apr 12, 2024

Hi @Kyle772

Have you been able to take a look at the feedback I've posted on the PR? I'd love to get this feature merged.

Wow I'm so sorry I don't ever check my github inbox lol. I'll try to address these changes sometime over the next couple of weeks. Sort of slammed at the moment but would love for this to get merged as I use my own fork on my sites

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants