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

Version 2 #328

Open
wants to merge 123 commits into
base: main
Choose a base branch
from
Open

Version 2 #328

wants to merge 123 commits into from

Conversation

bogdan
Copy link
Owner

@bogdan bogdan commented Nov 9, 2024

Major Changes

  1. Support Ruby open end Range for range: true filters
  2. Modern modular CSS class names
  3. Use HTML5 input types: date, datetime-local, number
  4. Enhanced built-in partials for maximum customization of the UI

Backward compatibility

Version-2 is has some breaking changes to ensure best implementation of modern technologies in the gem.

Migration Guide

TODO

[X] Integrate into datagrid-demo
[X] Integrate into 7F app
[ ] Update wiki

Fixes #325.

@zhuravel
Copy link
Contributor

Hey @bogdan,

I noticed a potential breaking change in how grid attributes are handled between v1 and v2.

Issue: The grid reconstruction from saved attributes now behaves differently, which is causing test failures.
Our test verifies that a grid can be rebuilt from its attributes while maintaining attribute consistency.
Example reproducing the behavior:

class ExampleCampaignsFilter < ApplicationGrid
  filter(:live_campaigns, :boolean, dummy: true)
  scope { [] }
end

# Initialize and reconstruct grid
original_grid = ExampleCampaignsFilter.new({})
attributes_saved_in_database = original_grid.attributes.slice(:live_campaigns)
restored_grid = ExampleCampaignsFilter.new(attributes_saved_in_database)

Datagrid v1:

original_grid.live_campaigns # => false
restored_grid.live_campaigns # => false

Attributes match, both grids have attributes: {:order=>nil, :descending=>nil, :live_campaigns=>false}

Datagrid v2:

original_grid.live_campaigns # => nil
restored_grid.live_campaigns # => false

Attributes differ:

original_grid.attributes # => {:order=>nil, :descending=>nil, :live_campaigns=>nil}
restored_grid.attributes # => {:order=>nil, :descending=>nil, :live_campaigns=>false}

Could you confirm if this is intended behavior in v2?

@bogdan
Copy link
Owner Author

bogdan commented Nov 13, 2024

Could you confirm if this is intended behavior in v2?

This is a regression. boolean filter can have only 2 states based on UI: checked and unchecked. That is why it can have only false/true as filter values.

Fixed in 12de0d4.

@bogdan
Copy link
Owner Author

bogdan commented Nov 14, 2024

@le0pard I wonder if you are using datagrid anywhere in your project and if you can help me to beta test the Version 2 and upgrade guide. I would appreciate early adoption from the branch and feedback.

@zhuravel
Copy link
Contributor

@bogdan I think 5a0d966 introduces a bug that our tests caught.

Code that is passing before this commit but not after:

class ParentGrid < ApplicationGrid
  filter(:created_at, :date, range: true, header: 'Period', default: :last_year_with_today) do |value, scope, grid|
    scope
  end

  def last_year_with_today
    1.day.since(1.year.ago)..Date.current
  end
end

class ChildGrid < ParentGrid
  scope do
    []
  end
end

ChildGrid.new.assets

# Before this commit - returns []
# 
# After this commit - raises error:
# 
# undefined method `call' for nil (NoMethodError)
# 
#         @driver ||= Drivers::AbstractDriver.guess_driver(scope_value.call).new

@le0pard
Copy link

le0pard commented Nov 14, 2024

@bogdan sorry, project which used gem already closed :( I can try to play with it in new rails project

@bogdan
Copy link
Owner Author

bogdan commented Nov 14, 2024

@le0pard this wouldn't be as valuable as you might not experience upgrading and non-standard use cases, but you may try if you have some free time.

@bogdan
Copy link
Owner Author

bogdan commented Nov 14, 2024

@bogdan I think 5a0d966 introduces a bug that our tests caught.

Fixed a80b0bc

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

Successfully merging this pull request may close these issues.

HTMLs for range filters don't conform to the HTML standards
3 participants