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

Add a settings popover to exclude post-types from tracking #51

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

aristath
Copy link
Member

@aristath aristath commented Jul 8, 2024

Context

This PR adds a popover for settings, to allow excluding post-types from tracking.

The settings popover was added to the header, right next to the months selector:

Screenshot 2024-07-08 at 11 11 12 AM

Initially, I wanted to add a setting to select which post-types will be included, however that would lead to lots of complications if post-types get added later. With that in mind, it seemed a better option to add a setting to exclude post-types.
To avoid designing a new page etc, it seemed better to reuse the popover API we already implement - especially since we only have 1 option. If in the future we add more options, we may want to rethink that approach

Screenshot 2024-07-08 at 11 42 09 AM

Summary

This PR can be summarized in the following changelog entry:

  • Add settings popover to exclude post-types from tracking.

Relevant technical choices:

  • Used an AJAX call to save the options, to avoid reloading the page.

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities.
  • I have added unit tests to verify the code works as intended.
  • I have checked that the base branch is correctly set.

@aristath aristath requested a review from jdevalk July 8, 2024 08:21
Copy link
Contributor

github-actions bot commented Jul 8, 2024

Test on Playground
Test this pull request on the Playground or download the zip

@jdevalk
Copy link
Member

jdevalk commented Jul 8, 2024

I've been discussing this with Marieke. We'd like to change it; instead of making it "Exclude", I think we should make it "Include" and we should default to Posts and Pages "on" and the rest "off".

classes/popups/class-settings.php Outdated Show resolved Hide resolved
classes/popups/class-settings.php Outdated Show resolved Hide resolved
assets/js/settings.js Outdated Show resolved Hide resolved
assets/js/settings.js Outdated Show resolved Hide resolved
assets/js/settings.js Outdated Show resolved Hide resolved
@jdevalk
Copy link
Member

jdevalk commented Jul 8, 2024

Also; hitting "Save" should trigger a page refresh, as it changes the data that should be displayed.

@jdevalk jdevalk merged commit 130c410 into develop Jul 8, 2024
18 checks passed
@jdevalk jdevalk deleted the settings/exclude-post-types branch July 8, 2024 09:52
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.

2 participants