bogdan / datagrid

Gem to create tables grids with sortable columns and filters
MIT License
1.02k stars 115 forks source link

Allow non-number characters in number filters #235

Closed gabebw closed 6 years ago

gabebw commented 6 years ago

I have a salary range filter in my app, and users often enter $100,000 or $50,000, which doesn't work. This change removes all non-number characters from a filter value before attempting to convert it to an integer.

One possible problem is that "3.4" would now become 34 instead of 3. I could easily change the regex to allow . if you'd prefer.

gabebw commented 6 years ago

I added a to_s because sometimes the value is already an integer (and thus .gsub crashes), though I'm not sure if that situation will occur outside of specs.

bogdan commented 6 years ago

I don't quite fine with this implementation.... I would rather consider :format option.... but I need to think about it first.

Please give some more time.

gabebw commented 6 years ago

I decided to implement this as a normal string filter, which I then post-process into a float:

filter :salary_minimum, header: 'Minimum salary (inclusive)' do |low|
  number = low.gsub(/[$,.]/, '').to_f
  where('salary >= ?', number)
end

filter :salary_maximum, header: 'Maximum salary (inclusive)' do |high|
  number = high.gsub(/[$,.]/, '').to_f
  where('salary <= ?', number)
end

I think that this PR is maybe not as generally useful as I thought it would be, so I'm closing it.

bogdan commented 6 years ago

Maybe the best approach would be to all allow range: true for the string filter type. In this case the interface will be better for the end user and you will only need that cleanup code inside filter block. I will think about that too.

bogdan commented 6 years ago

@gabebw I added range support for the string filter. You can now have better UI with:

filter(:salary, :string, range: true) do |range|
  where(salary: range.map {|x| x.tr('$,.').to_f
end

I didn't make the release yet because want to ensure it works for your env at least first.