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

Admin tables order - sorting [#2931] #3148

Merged
merged 25 commits into from
Feb 15, 2019
Merged

Admin tables order - sorting [#2931] #3148

merged 25 commits into from
Feb 15, 2019

Conversation

matisnape
Copy link
Collaborator

@matisnape matisnape commented Jan 1, 2019

References

Issue: Admin tables order #2931

Objectives

  • remove sort select for Budget Investments
  • implement sorting in columns: ID, Title, Supports
  • submit first OSS PR :D

Visual Changes

  • removed sort_by select
  • added arrow icon to indicate sorting direction

image

Notes

  • modified tests to support new sorting rules
  • fixed a bug with unsupported sort_by values (e.g. edited manually in the address field) - now the controller just ignores it and returns all investments
  • I've created 3 helper methods for sorting in Budget Investments Helper for the sake of this PR, but eventually I think it should become a separate helper or module for sorting, which would also contain the model method self.order_filter - let me know which strategy you would like me to follow.

This is my first ever external commitment and I don't have commercial experience, so I figure there is a lot to refactor. I'd be happy to do that after code review :) Just point everything I could improve and I'll do my best :) I'd also would like to implement sorting for other tables as well so I'd appreciate any guidance towards making this more reusable.

@microweb10 microweb10 added this to Doing in Roadmap Jan 2, 2019
@matisnape
Copy link
Collaborator Author

matisnape commented Jan 2, 2019

I think it's pretty much it. Let me know what you think so far :)

(Those failing tests are not related to my changes, though)

matisnape and others added 25 commits January 10, 2019 15:28
We were assuming the translation and the column name were related.
It's OK ot use single quotes inside a string with double quotes in order
to avoid escape characters.
The string inside `params[:direction]` should already use lowercase
characters.
Using a simple ternary operator is usually fine; however, code combining
two ternary operator is a bit hard to follow.
Using a hash instead of an array of hashes makes accessing its keys and
values much easier.
Just the way is usually done in the rest of the code.
@voodoorai2000 voodoorai2000 moved this from Doing to Review in Roadmap Jan 14, 2019
@voodoorai2000 voodoorai2000 removed this from Review in Roadmap Jan 14, 2019
@javierm
Copy link
Member

javierm commented Jan 15, 2019

@matisnape Thank you very much for this pull request! 🏆

It looks like there are conflicts with our current master branch. Since you've so generously worked on this issue during your holidays ❤️, we felt taking up more of your time by asking you to deal with these conflicts (which were caused by us) wouldn't be so kind from our side.

So we've opened a new branch rebasing your changes against current master and fixing a couple of minor issues (hope you don't mind! 🙏): https://github.com/consul/consul/tree/rebased_budget_investments_sorting_columns

In order to update this pull request, run the following commands:

git remote add upstream https://github.com/consul/consul  
git fetch upstream rebased_budget_investments_sorting_columns
git checkout budget_investments_sorting_columns
git reset --hard upstream/rebased_budget_investments_sorting_columns
git push -f

Then we can merge it into master 😄.

@matisnape
Copy link
Collaborator Author

@javierm
Sure, I will get to it this week :)

I would like to continue working on adding sorting to other tables and open another PR after this one is merged. Do you have any suggestions on how to make the sorting more reusable?

@javierm
Copy link
Member

javierm commented Jan 15, 2019

@matisnape That would be awesome! Thank you so much 🙏!

In order to make the code more reusable, you can try one of these approaches:

  1. Make all table headers on this table sortable (which might prove quite a challenge in some cases)
  2. Add sortable headers to another table

Once we do similar tasks in a couple of places, it's easier to find ways to reuse the code 😄.

@matisnape
Copy link
Collaborator Author

@javierm

Add sortable headers to another table

You mean new table in database?

@javierm
Copy link
Member

javierm commented Jan 15, 2019

I mean another table (<table> HTML tag) in the admin section 😉.

@matisnape
Copy link
Collaborator Author

@javierm done! :)

@javierm javierm merged commit b330de0 into consuldemocracy:master Feb 15, 2019
@javierm
Copy link
Member

javierm commented Feb 15, 2019

@matisnape Thanks a lot! 🎉

@matisnape
Copy link
Collaborator Author

I'll start working on adding sorting in other places :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants