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

Mark processed tables #121

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Mark processed tables #121

merged 2 commits into from
Dec 21, 2023

Conversation

alijsh
Copy link
Contributor

@alijsh alijsh commented Dec 21, 2023

Make sure each table is processed once by adding the class name "table-processed". This prevents re-processing tables and attaching repeated arrows and click events in scenarios where new tables are dynamically added to a web page/application. Now, only new tables are made sortable.

Make sure each table is processed once by adding the class name "table-processed". This prevents re-processing tables and attaching repeated arrows and click events in scenarios where new tables are dynamically added to a web page/application. Now, only new tables are made sortable.
@LeeWannacott
Copy link
Owner

LeeWannacott commented Dec 21, 2023

@alijsh Thanks, this should help with when someone has the browser extension installed and visiting a site that is running table-sort-js like the documentation demo; (it currently has this problem if you have the extension installed).

I probably would of written it like this:

   if !(sortableTable.classList.contains("table-processed")) {
         sortableTable.classList.add("table-processed");
    } else {
        return;
    }

Not that it really matters in this instance, This is because else is a fall through for any condition that you might not have thought about:

or could of put it up further (this way it only adds one line):

  for (let table of getTagTable) {
    if (table.classList.contains("table-sort")) && !(table.classList.contains("table-processed")) {
      makeTableSortable(table);
    }
  }
 makeTableSortable(sortableTable){
     sortableTable.classList.add("table-processed");
     ....
}

Let me know if you want to make changes or want me to merge it as is.

Cheers.

@LeeWannacott LeeWannacott merged commit 30c6cca into LeeWannacott:master Dec 21, 2023
1 check passed
@alijsh
Copy link
Contributor Author

alijsh commented Dec 21, 2023

@LeeWannacott I think the second approach is better (checking "table-processed" and "table-sort" classes in the same condition and then, adding "table-processed" class to new tables). I made the changes. Please check it.

@LeeWannacott
Copy link
Owner

@alijsh Yep, looks good to me.

@alijsh alijsh deleted the patch-1 branch December 21, 2023 09:21
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

2 participants