Skip to content

Conversation

@thednp
Copy link
Contributor

@thednp thednp commented Oct 1, 2020

PR for Issue #30288

Summary of Changes

As explained with the issue at hand, the choices.js provides support for a render limit of the options and a search results limit, the first is much more important.

  • build/media_source/system/js/fields/joomla-field-fancy-select.w-c.es6.js now can use render-limit and results-limit attributes to set searchResultLimit and renderChoiceLimit options for choices.js;
  • libraries/src/Form/Field/ListField.php now can properly add the renderLimit and resultsLimit attributes from layout;
  • layouts/joomla/form/field/list-fancy-select.php can now add the render-limit and results-limit HTML attributes to the element from the layout set renderLimit and resultsLimit field properties.

Alternative solution
Since the issue we're trying to solve is the slow initialization for large lists, we can simply replace searchResultLimit with renderChoiceLimit with the same value of 10 in build/media_source/system/js/fields/joomla-field-fancy-select.w-c.es6.js. The user would expect a search to take some time, but an initialization cannot take 2-3 seconds per field. This however won't give developers much flexibility.

Testing Instructions

  • Open any extensionName.xml and add this field in any <fieldset>
    <field
      name="fancy-select"
      type="List"
      label="List Demo"
      default="5"
      renderLimit="5"
      resultsLimit="15"
      layout="joomla.form.field.list-fancy-select">
      <option value="0">zr</option>
      <option value="1">zr 1</option>
      <option value="2">zr 2</option>
      <option value="3">zr 3</option>
      <option value="4">zr 4</option>
      <option value="5">zr 5</option>
      <option value="6">zr 6</option>
      <option value="7">zr 7</option>
      <option value="8">zr 8</option>
      <option value="9">zr 9</option>
      <option value="10">ze 10</option>
      <option value="11">ze 11</option>
      <option value="12">ze 12</option>
      <option value="13">ze 13</option>
      <option value="14">ze 14</option>
      <option value="15">ze 15</option>
      <option value="16">ze 16</option>
      <option value="17">ze 18</option>
      <option value="18">ze 18</option>
      <option value="19">ze 19</option>
      <option value="20">zt 20</option>
      <option value="21">zt 21</option>
      <option value="22">zt 22</option>
      <option value="23">zt 23</option>
      <option value="24">zt 24</option>
      <option value="25">zt 25</option>
      <option value="26">zt 26</option>
      <option value="27">zt 27</option>
      <option value="28">zt 28</option>
      <option value="29">zt 29</option>
      <option value="30">zt 30</option>
    </field>
  • save and go to that extension config
  • open the field dropdown, there should be 5 options as configured;
  • type in 'Z' in the search box of the field, you should get 15 results as configured.

Actual result BEFORE applying this Pull Request

Large lists are initialized extremely slow, also the initial resultsLimit of 10 was not enough. Imagine more than 10 Roboto font family styles, or much more articles that contain a certain keyword / combination of keywords.

Expected result AFTER applying this Pull Request

Much faster initialization, also more flexible PHP extend possibilities.

Documentation Changes Required

Yes.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Oct 1, 2020
@thednp thednp changed the title Set proper limits for joomla-fancy-select layout [4.0] Set proper limits for joomla-fancy-select layout Oct 1, 2020
@Quy
Copy link
Contributor

Quy commented Oct 1, 2020

Please fix coding style: https://ci.joomla.org/joomla/joomla-cms/35972/1/8

@thednp
Copy link
Contributor Author

thednp commented Oct 1, 2020

@Quy I don't know how to validate that issue, it might be unrelated.

@thednp thednp requested a review from Quy October 1, 2020 19:36
@mqueme
Copy link

mqueme commented Oct 17, 2020

I tested, 20ms improvement in loading time. but when I save the module settings it doesn't save the item when set to blank. and list still shows all items

@mqueme
Copy link

mqueme commented Oct 17, 2020

I have tested this item 🔴 unsuccessfully on b857e0d

I tested, 20ms improvement in loading time. but when I save the module settings it doesn't save the item when set to blank. and list still shows all items


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

@thednp
Copy link
Contributor Author

thednp commented Oct 17, 2020

@mqueme

when I save the module settings it doesn't save the item when set to blank. and list still shows all items

it might be unrelated, but to be sure I will test this as soon as I get some time.

Thanks

@thednp
Copy link
Contributor Author

thednp commented Oct 19, 2020

@mqueme I tested the mod_articles_news with the above sample field and I confirm that after I save module settings with blank value, it will reset to first item in the list, which is something probably choices.js script does because the rendered select on which the script initialize also sets the first item as checked.

I believe this is a non-issue. Anyone else have an opinion on this?

@Fedik
Copy link
Member

Fedik commented Oct 19, 2020

I confirm that after I save module settings with blank value, it will reset to first item in the list

it is correct behavior of <select> form input, you have first item <option value="0">zr</option> that will be selected by default if there no value

@thednp
Copy link
Contributor Author

thednp commented Oct 19, 2020

@mqueme

That behavior is expected especially if you want your form field to force the end used to use a value. For instance SCSS mixins/styles cannot work with blank values, $font-size or $line-height must be valid.

In other cases you can work with blank values for your form fields, you can simply add a new <option value="">None</option> it will be able to do that.

It's up to the developer to decide on his/her extension how the form field of type="List" should work.

@thednp
Copy link
Contributor Author

thednp commented Oct 20, 2020

Do you guys think we need to implement choices.js removeItemButton option for cases where the null value is an option? (<option value="">None</option>)

I'm also thinking of a way to check the first CHOICE and set this instance option automatically based on the value. (NULL => true, notNULL => false), what do you guys think?

@mqueme
Copy link

mqueme commented Oct 20, 2020

I think it should accept null values, the current option has an x to remove the selected value which implies clearing the field. But saving it put the value back which makes it very confusing.

@thednp
Copy link
Contributor Author

thednp commented Oct 21, 2020

@mqueme that is why I proposed in my previous post implementation of removeItemButton from choices.js.

@Fedik
Copy link
Member

Fedik commented Oct 21, 2020

I not very like that idea. choices.js mimic behavior of <select> input.
<select> does not have any "removeItem" option, however if someone need it in custom field, he/she always can create a custom layout and do whatever he/she want

@thednp
Copy link
Contributor Author

thednp commented Oct 21, 2020

@Fedik understood. However setting proper limits is pretty important as explained above.

@thednp
Copy link
Contributor Author

thednp commented Mar 5, 2021

@Quy @Fedik any chance of ever merging this?

@Fedik
Copy link
Member

Fedik commented Mar 5, 2021

if you will use data- attribute (follow #27212, #29252 ), and drop getLayoutData hacky thing, then maybe I try to test

@thednp
Copy link
Contributor Author

thednp commented Mar 5, 2021

Why didn't you say so? Should I follow up on @HLeithner commit? it seems logical to me.

@thednp
Copy link
Contributor Author

thednp commented Mar 5, 2021

@Fedik make layouts/joomla/form/field/list-fancy-select.php work with data-renderLimit and data-searchLimit instead?

@thednp
Copy link
Contributor Author

thednp commented Mar 5, 2021

This PR is obsolete. Will commit a new one soon.

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

Labels

Documentation Required NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants