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 filter by Tag to mod_article_category #16945

Merged
merged 7 commits into from Jul 29, 2017

Conversation

Projects
None yet
10 participants
@hans2103
Contributor

hans2103 commented Jul 3, 2017

Pull Request for missing feature

Summary of Changes

This Pull Request add the option to filter by tag.

  • When you create a menu item for Category Blog you can select both Category and Tag.
  • When you create the module mod_article_category you cannot filter by Tag.
    This pull request adds the ability to filter by tag too.

Testing Instructions

  • Have com_content items with tags
  • Create a module mod_article_category
    • assign to a visible module position
    • select category to filter the content
    • show X amount of items to display
    • select tag to filter... oh boy... there is no tag filter

apply patch

  • Refresh backend
  • Open created module
    • select tag to filter... yes... there is the option to filter by tag

Expected result

Before patch

  • Refresh frontend and notice X amount of items from a selected category.

After patch

  • Refresh frontend and notice X amount of items from a selected category and filtered by tag

Documentation Changes Required

Be sure you have content items with tags assigned to them.

@allrude

This comment has been minimized.

Show comment
Hide comment
@allrude

allrude Jul 3, 2017

I have tested this item successfully on eadf35a

I tested this extra option successfully, nice to have this option, Thanks Hans


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

allrude commented Jul 3, 2017

I have tested this item successfully on eadf35a

I tested this extra option successfully, nice to have this option, Thanks Hans


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 3, 2017

I have tested this item successfully on eadf35a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

franz-wohlkoenig commented Jul 3, 2017

I have tested this item successfully on eadf35a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 3, 2017

RTC after two successful tests.

franz-wohlkoenig commented Jul 3, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Jul 3, 2017

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 3, 2017

Contributor

I fixed the codestlye issues

Contributor

brianteeman commented Jul 3, 2017

I fixed the codestlye issues

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 3, 2017

@brianteeman should we test again or set RTC?

franz-wohlkoenig commented Jul 3, 2017

@brianteeman should we test again or set RTC?

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jul 3, 2017

Contributor

it was only codestyle so one more test will be fine but it shouldnt have been set to rtc with those errors

Contributor

brianteeman commented Jul 3, 2017

it was only codestyle so one more test will be fine but it shouldnt have been set to rtc with those errors

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 3, 2017

I have tested this item successfully on 839e617


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

franz-wohlkoenig commented Jul 3, 2017

I have tested this item successfully on 839e617


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig commented Jul 3, 2017

RTC.

@infograf768 infograf768 added this to the Joomla 3.7.4 milestone Jul 6, 2017

@rdeutz rdeutz added the New Feature label Jul 6, 2017

@rdeutz rdeutz modified the milestones: Joomla 3.8.0, Joomla 3.7.4 Jul 6, 2017

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jul 25, 2017

Member

This needs to be synced with staging before I can merge it. We added a state filter to stop loading tags in the modules with 008e5cd and that is going to conflict with this PR.

Member

mbabker commented Jul 25, 2017

This needs to be synced with staging before I can merge it. We added a state filter to stop loading tags in the modules with 008e5cd and that is going to conflict with this PR.

@hans2103

This comment has been minimized.

Show comment
Hide comment
@hans2103

hans2103 Jul 28, 2017

Contributor

@mbabker synched and ready to be tested again. Added php logic to conditional load tags or not. Depending if any tag is selected to filter.

Contributor

hans2103 commented Jul 28, 2017

@mbabker synched and ready to be tested again. Added php logic to conditional load tags or not. Depending if any tag is selected to filter.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Jul 28, 2017

Member

Looks fine, if someone can do a quick test with the added logic in place we should be good to merge this.

Member

mbabker commented Jul 28, 2017

Looks fine, if someone can do a quick test with the added logic in place we should be good to merge this.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig commented Jul 28, 2017

will test now.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Jul 28, 2017

I have tested this item successfully on d1017ad


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

franz-wohlkoenig commented Jul 28, 2017

I have tested this item successfully on d1017ad


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Jul 29, 2017

Contributor

I have tested this item successfully on d1017ad


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

Contributor

alikon commented Jul 29, 2017

I have tested this item successfully on d1017ad


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16945.

@mbabker mbabker merged commit 5abadbb into joomla:staging Jul 29, 2017

5 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot removed the RTC label Jul 29, 2017

izharaazmi added a commit to izharaazmi/joomla-cms that referenced this pull request Jul 29, 2017

Merge branch 'staging' into menu-preset-features
* staging: (35 commits)
  Add filter by Tag to mod_article_category (#16945)
  Change links
  Implement component params for fieldgroups (#17317)
  Moved JLanguageMultilang::isAdminEnabled() to JModuleHelper::isAdminMultilang() (#17314)
  Fix tests
  Namespace sodium cipher, use compat API
  Fix namespace use and class casing
  Various doc block fixes and removing unneeded imports
  Class mappings for internal classes that apparently aren't internal
  Don't load unexisting paths
  Cleanup and optimization in FinderIndexerDrivers (#13511)
  Fix deleted files listing
  Deleted files updated
  Rename the document renderer base class
  Split feed data object classes to separate files
  Correct class name logic
  Deleted files for #17278
  Namespace feed (#17278)
  [CS] Required=true (#17313)
  required = false is nnot required (#17309)
  ...

mbabker added a commit that referenced this pull request Jul 31, 2017

Admin Menu custom presets, import/export a preset into/from database …
…driven menu (#16451)

* Implement preset xml and the menuHelper class to load them.

* Export a menu (menutype) as a preset xml

* Import/load a preset xml into a database driven menu (menutype)

* Menu Tree and Node classes implemented for menu module menu item hierarchy.

* Update menu module to use new Menu Tree and Node class. Move the HTML rendering to layout files to facilitate template overrides.

* Remove unused files.

* CodeStyle fixes

* Include exception message string in error message.

* Add filter by Tag to mod_article_category (#16945)

* add filteroption to select tag to filter

* all php logic to get selected filter option

* add nice lines to separate fitler by tag from the rest.

* fix code style

* conditional load tags. Only load when filter is used

* File mode fixed
@pepperstreet

This comment has been minimized.

Show comment
Hide comment
@pepperstreet

pepperstreet Oct 10, 2017

Hello, in the XML... shouldn't the tag-filter parameter get the "multiple" option?
Comparable to the improved Blog MenuItem parameter in this issue #18021 and PR #18234

pepperstreet commented Oct 10, 2017

Hello, in the XML... shouldn't the tag-filter parameter get the "multiple" option?
Comparable to the improved Blog MenuItem parameter in this issue #18021 and PR #18234

@hans2103

This comment has been minimized.

Show comment
Hide comment
@hans2103

hans2103 Oct 10, 2017

Contributor

@pepperstreet please create a new pull request. This one has already been merged and closed.

Contributor

hans2103 commented Oct 10, 2017

@pepperstreet please create a new pull request. This one has already been merged and closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment