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

Better support for modification of HTML of existing lists #652

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pdw-mb
Copy link

@pdw-mb pdw-mb commented Mar 1, 2019

Modification of the HTML in existing lists is not currently well supported. The reIndex function will reinitialise the list from the current HTML, but this does not interact well with search or pagination functions, as whenever reIndex() is called, it will truncate the list to include only the visible rows.

This is discussed here: #86 (comment)

This PR provides an alternative approach.

addItem() allows you to provide the DOM object for an element that has been added which will be added to the list.
refresh() will reload all existing rows from their corresponding HTML elements, which is useful of their content has been modified.
sort() called with no arguments will re-apply the last search. This allows newly inserted or modified rows to be shown in the correct order.

pdw-mb and others added 4 commits February 28, 2019 23:48
This is different to reIndex() which will recreate the list from the
current visible rows.  reIndex() will remove rows that are not currently
shown due to filtering or pagination.
@codecov-io
Copy link

Codecov Report

Merging #652 into master will decrease coverage by 1.59%.
The diff coverage is 26.31%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #652     +/-   ##
=========================================
- Coverage   94.74%   93.14%   -1.6%     
=========================================
  Files          19       19             
  Lines         799      817     +18     
  Branches      187      190      +3     
=========================================
+ Hits          757      761      +4     
- Misses         29       41     +12     
- Partials       13       15      +2
Impacted Files Coverage Δ
src/index.js 95.37% <22.22%> (-4.02%) ⬇️
src/sort.js 90.14% <28.57%> (-6.79%) ⬇️
src/item.js 94.59% <33.33%> (-5.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52afe2d...6740006. Read the comment docs.

@mauricekindermann
Copy link

This saved my life. I was banging my head against the wall trying to figure out how on earth I could work around the reIndex() issue when using a couple fairly complex filter() functions which are tied to other libraries.

I thought maybe select2 or swal were breaking my code, then I realised reIndex() was truncating my data, then I realised this is a known issue and has no fix in sight, untli I finally found the actual code here:
84a9ea1

I'm intentionally leaving the link and a long winded expliation above incase someoen else finds themselves in my position

@vanboom
Copy link

vanboom commented Feb 6, 2022

Suggestion - refactor reIndex with this behavior rather than making another function. Imho, reIndex() should re-index the full HTML without the List.js specific filtering applied.

@vanboom
Copy link

vanboom commented Feb 6, 2022

Also for your consideration, your item reload function does not appear to load the new HTML. This logic will fetch the fresh HTML from the DOM, reload the values and achieve the desired result.

    this.reload = function() {
      // reload the items from source HTML (not the templater)
      var itemSource = document.getElementById(item.elm.id);
      if (itemSource) {
        var newItem = itemSource.cloneNode(true);
        this.elm = newItem;
        // reconstitute the values
        var values = list.templater.get(item, initValues);
        this.values(values);
      }
    };

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

5 participants