KamandPrompt / CodeManiacs

IIT Mandi Online Judge
MIT License
23 stars 58 forks source link

Sort feature added in all tables #164

Closed pankajkumarbij closed 3 years ago

pankajkumarbij commented 3 years ago

Hello, I solved issue no #99

Now we can sort the entities by clicking on the header of that column.

If you create a new table in the future then add a class like class="sortable" in that table then the sort feature is also working for that table.

Screenshot from 2020-12-18 16-51-58

Thanks.

dheeraj135 commented 3 years ago

@vsvipul Please review this PR.

pranshukharkwal commented 3 years ago

Hi @pankajkumarbij Good work on this, but I think there are some unwanted changes in the design of the tables See : (BEFORE) Screenshot from 2020-12-18 04-18-55 (AFTER) Screenshot from 2020-12-18 04-19-09

Can you investigate why this is happening and how could this be fixed?

pankajkumarbij commented 3 years ago

Now all the unwanted changes in the table are resolved. The sort feature is working very well. you can check by clicking on the table column's header. Now please review it.

Screenshot from 2020-12-18 16-51-58

pranshukharkwal commented 3 years ago

Screenshot from 2020-12-19 03-11-28 I am getting this error in the terminal when I try to run the code in this PR. @Signior-X could you please check if you too see a similar error?

pankajkumarbij commented 3 years ago

It works fine on my side. Please check again and accept PR.

dheeraj135 commented 3 years ago

Pranshu, the error says address is already in use. Try killing all npm/nodemon processes and try again. If this doesn't help, restart your pc.

On Sat, 19 Dec 2020, 03:12 pranshukharkwal, notifications@github.com wrote:

[image: Screenshot from 2020-12-19 03-11-28] https://user-images.githubusercontent.com/21126219/102664019-ef981f00-41a7-11eb-9f14-b535e5765572.png I am getting this error in the terminal when I try to run the code in this PR. @Signior-X https://github.com/Signior-X could you please check if you too see a similar error?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/KamandPrompt/CodeManiacs/pull/164#issuecomment-748333650, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG2G2BA37PX5CJ7CRWTUBWDSVPEF7ANCNFSM4UWPENPA .

Signior-X commented 3 years ago

@pranshukharkwal I tested this on my PC, working well. Do as suggested by Dheeraj. @pankajkumarbij this is working as expected, quite nice. Just one thing I noticed, hovering on the heading of the column doesn't make a feel that it is kind of button to sort.

image Like here hovering on the name doesn't feel like a button. Can you try to add a tooptip on the column headings wherever they are needed or check if there is something kind of in the library you used.

@pranshukharkwal I also need your suggestion in this regard.

pranshukharkwal commented 3 years ago

@Signior-X @pankajkumarbij Yes, I too think that something should be there to indicate that these columns could be sorted. Currently it just looks like a table.

pankajkumarbij commented 3 years ago

There is two types of changes I can do:-

  1. Feel the table's column header like a button or link.
  2. I can use a Sort Icon with a heading and also feel like a button(given below picture)

Screenshot from 2020-12-20 10-05-44

Which one you want please let me know after that I will update this PR.

pranshukharkwal commented 3 years ago

Second option LGTM. @Signior-X what do you say?

pankajkumarbij commented 3 years ago

Please check now table headings look like a sortable button.

Screenshot from 2020-12-20 10-05-44

check and let me know if any mistakes.

pankajkumarbij commented 3 years ago

Please review this PR.

pranshukharkwal commented 3 years ago

@pankajkumarbij I have an exam tomorrow. I will review this pr after that. It will be done by tomorrow

pranshukharkwal commented 3 years ago

@Signior-X The changes look good to me. Please review the PR and merge it if it is all good.

Signior-X commented 3 years ago

@pankajkumarbij Awesome work done... to the point! Keep up the good work. @pranshukharkwal mergung this one