Skip to content

Comments

[4.0] Prefill tags with most used items#31481

Merged
wilsonge merged 6 commits intojoomla:4.0-devfrom
HLeithner:j4/tags
Dec 18, 2020
Merged

[4.0] Prefill tags with most used items#31481
wilsonge merged 6 commits intojoomla:4.0-devfrom
HLeithner:j4/tags

Conversation

@HLeithner
Copy link
Member

Pull Request for Issue #31348 .

Summary of Changes

Pre-fill the tags field with (default) 30 items based on the most used count.
Also reduce the min term length to 1, if there is a performance issue the site owner has to tweak this value.

Testing Instructions

  • Open an article
  • Look at the tags field, you will see no tags
  • Create 40 tags (or reduce the configuration entry "Initial number of shown tags" in com_tags config to a lower number
  • Open an article
  • You will see 30 tags
  • Create several articles
  • Add the always the same 10 tags
  • You will always see the 10 tags and 20 additional tags
  • Also test non ajax mode

Actual result BEFORE applying this Pull Request

The tags field was empty

Expected result AFTER applying this Pull Request

You have some (hopefully) tags to select.

Documentation Changes Required

N/A

Thanks to @bembelimen for helping

@richard67
Copy link
Member

@wilsonge
Copy link
Contributor

wilsonge commented Nov 26, 2020

So looks like this blocker was intended. The remoteSearch option seems to have been added as part of the move to the choices.js element #22263 - first of all I guess it's worth asking whether we should roll back some of that code rather than adding in additional queries

I definitely don't see the need to allow people to select the number of items returned? This seems like our favourite parameter creep

Finally I think reducing the min term length should be done outside the scope of this PR. There's no need to really change that as part of this change

Good work at figuring out this one though!

@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Dec 1, 2020
@HLeithner
Copy link
Member Author

@wilsonge I updated the PR by reverting the default min length and removed the limit parameter

catch (\RuntimeException $e)

// Only execute the query if we need more tags not already loaded by the $preQuery query
if (!$isRemoteSearch || $prefillLimit > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this code always going to run because $prefillLimit is set to 30 (and even in the previous version of your code if this was a parameter unless that param had been set to 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

$prefiilLimit is set in line 244

@punambaravkar
Copy link

I have tested this item ✅ successfully on aea7162

some tags to select.


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

@vaibhavsTekdi
Copy link
Contributor

I have tested this item ✅ successfully on aea7162

Yes, After adding the patch, it works correctly & I can see the previously added tags in the tags filter on the article list view.


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

@HLeithner HLeithner added the RTC This Pull Request is Ready To Commit label Dec 5, 2020
@wilsonge wilsonge merged commit f2a29d3 into joomla:4.0-dev Dec 18, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 18, 2020
@wilsonge
Copy link
Contributor

Finally tested this and it's completely fine. Thanks!

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.

7 participants