-
Notifications
You must be signed in to change notification settings - Fork 12
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
templates/base.html.twig
Outdated
{% 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> |
There was a problem hiding this comment.
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?
templates/base.html.twig
Outdated
@@ -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" /> |
There was a problem hiding this comment.
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?
templates/contests_view.html.twig
Outdated
@@ -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> |
There was a problem hiding this comment.
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?
templates/home.html.twig
Outdated
@@ -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> |
There was a problem hiding this comment.
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.
Sure I think i have made some mistakes and will try to change it. |
There was a problem hiding this 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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
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.
Hi @fatemaker254 how are you going with this? Do you need anything to help? |
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. |
All the tables in the project is added with the sorting feature and now user can sort the tables in different orders.