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

Refactor. #240

Merged
merged 2 commits into from
Jul 16, 2015
Merged

Refactor. #240

merged 2 commits into from
Jul 16, 2015

Conversation

ryanfox1985
Copy link
Contributor

Hi I did my first refactor.

Can you check if fine this file?

lib/wice/helpers/wice_grid_view_helpers.rb
development...ryanfox1985:development#diff-a2fdf58bd2318a96538f29bab5343700L156

Regards

@leikind
Copy link
Owner

leikind commented Jul 16, 2015

filter_for is a helper which renders external filters. http://wicegrid.herokuapp.com/detached_filters

A external filter is defined by a key. If no such filter exists, it throws an exception, but if a grid has no records to show, no filters should be shown and we relax this rule by NOT throwing exceptions in this case

@leikind
Copy link
Owner

leikind commented Jul 16, 2015

if @return_empty_strings_for_nonexistent_filters then no exception should be thrown

@leikind
Copy link
Owner

leikind commented Jul 16, 2015

You have changed the arity of #find_one_for from 1 to 2, but you haven't changed the method itself. There will be an error

@ryanfox1985
Copy link
Contributor Author

Ok I understant what means, but can you check if my refactor is fine?

if rendering.find_one_for(:in_html) { |column| column.detach_with_id }
grid.output_buffer.return_empty_strings_for_nonexistent_filters = true
end
rendering.find_one_for(:in_html, true) { |column| column.detach_with_id }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will cause an error. Wice::GridRenderer#find_one_for has the arity of 1.
I think you want to pass true to the output_buffer, but rendering is a different object, it is not GridOutputBuffer where #filter_for is.

@ryanfox1985
Copy link
Contributor Author

Ok I understand I rollback some changes.

leikind added a commit that referenced this pull request Jul 16, 2015
@leikind leikind merged commit ca64889 into leikind:development Jul 16, 2015
@leikind
Copy link
Owner

leikind commented Jul 16, 2015

merged. Running my test suite

@ryanfox1985
Copy link
Contributor Author

Fine.

2015-07-16 11:47 GMT+02:00 Yuri Leikind notifications@github.com:

merged. Running my test suite


Reply to this email directly or view it on GitHub
#240 (comment).

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.

None yet

2 participants