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

The sorting feature is added to all the tables #60

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

Conversation

fatemaker254
Copy link

All the tables in the project is added with the sorting feature and now user can sort the tables in different orders.

Copy link
Member

@samwilson samwilson left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

It looks like this PR needs a few tweaks to make it work.

{% endblock %}
{% block scripts %}
<script type="text/javascript" src="//tools-static.wmflabs.org/cdnjs/ajax/libs/jquery/1.12.0/jquery.min.js"></script>
<script type="text/javascript" src="//tools-static.wmflabs.org/cdnjs/ajax/libs/twitter-bootstrap/3.3.6/js/bootstrap.min.js"></script>
<script src="sortable.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't exist. Did you forget to commit it? Or should this be referencing a file in cdnjs?

@@ -10,11 +10,17 @@
</title>
{% block stylesheets %}
<link rel="stylesheet" href="//tools-static.wmflabs.org/cdnjs/ajax/libs/twitter-bootstrap/3.3.6/css/bootstrap.min.css" />
<link rel="stylesheet" href="//tools-static.wmflabs.org/cdnjs/ajax/libs/twitter-bootstrap/3.3.6/css/bootstrap.min.css" />
Copy link
Member

Choose a reason for hiding this comment

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

This line duplicates the one before it. Did you mean to link to a different stylesheet?

@@ -51,7 +51,7 @@
<div id="scores">
<h3>{{ msg('scores') }}</h3>
{% if can_view_scores %}
<table class="table">
<table class="table" class="sortable-theme-bootstrap" data-sortable>
Copy link
Member

Choose a reason for hiding this comment

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

There's already a class attribute; any new classes should be added to that.

Also, this table is the only one with the theme applied; is that intentional?

@@ -29,7 +29,7 @@
3) the total number of constructive <em>Contributions</em> of all sorts.
</p>

<table class="table" role="table">
<table class="table" role="table" data-sortable>
Copy link
Member

Choose a reason for hiding this comment

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

No need for two spaces here.

@fatemaker254
Copy link
Author

Sure I think i have made some mistakes and will try to change it.

@fatemaker254 fatemaker254 reopened this Mar 16, 2023
Copy link
Member

@samwilson samwilson left a comment

Choose a reason for hiding this comment

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

Does this work for you, to sort table rows? It appears to be using the wrong library.

{% endblock %}
{% block scripts %}
<script type="text/javascript" src="//tools-static.wmflabs.org/cdnjs/ajax/libs/jquery/1.12.0/jquery.min.js"></script>
<script type="text/javascript" src="//tools-static.wmflabs.org/cdnjs/ajax/libs/twitter-bootstrap/3.3.6/js/bootstrap.min.js"></script>
<script src="https://cdn.jsdelivr.net/npm/sortablejs@1.14.0/Sortable.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

This (which is https://github.com/SortableJS/Sortable ) doesn't seem to be a table-sorting library, but rather is for sorting lists. The one I was suggesting is https://github.com/HubSpot/sortable

Libraries should also be loaded from the tools-static.wmflabs.org/cdnjs service.

Copy link
Author

Choose a reason for hiding this comment

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

Actually it was working for me. But will surely look into this

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry perhaps I'm testing it wrongly.

I was expecting to see sorting icons on the table headers, is that right? I don't see anything like that, and there are no errors in the JS console.

@@ -9,11 +9,12 @@
{{ msg('app-title') }}
</title>
{% block stylesheets %}
<link rel="stylesheet" href="//tools-static.wmflabs.org/cdnjs/ajax/libs/twitter-bootstrap/3.3.6/css/bootstrap.min.css" />
<link rel="stylesheet" href="//tools-static.wmflabs.org/cdnjs/ajax/libs/twitter-bootstrap/3.3.6/css/bootstrap.min.css" />
Copy link
Member

Choose a reason for hiding this comment

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

No need for the extra spaces at the end of the line.

@samwilson
Copy link
Member

Hi @fatemaker254 how are you going with this? Do you need anything to help?

@fatemaker254
Copy link
Author

Hi @samwilson actually last few days i was busy with my Gsoc proposal so i was not able to put that much effort in this. But now i will again start working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants