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

[4.0] Prefill tags with most used items #31481

Merged
merged 6 commits into from
Dec 18, 2020
Merged

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.

None yet

7 participants