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

allow super index to be called #165

Closed
wants to merge 2 commits into from

Conversation

arthurtalkgoal
Copy link

No description provided.

@schneems
Copy link
Member

I'm generally 👎 on inheriting controller code for public facing route options i.e. show, index, etc. This will fail if there's any kind of render or redirect in the other index action (double redirect error).

Tests are failing. What's your use case here?

@arthurtalkgoal
Copy link
Author

Hi @schneems ,

The inherited def index making it impossible to use the wick in the existing controllers. it is very troublesome. All the other methods are very well.

I will fix the Travis, the codes is fine in my local controllers which has def index

@schneems
Copy link
Member

making it impossible to use the wick in the existing controllers. it is very troublesome

What is your specific use case? What are you trying to do? What action exists in the index action you're trying to inherit?

@arthurtalkgoal
Copy link
Author

I override other plugins setup to step by step.   One is spree.

The spree admin countries use index method.

And using the index, edit, update show method name usually got conflict with other gems.   If it is configureable or use some specific names
, then the stepbystep flow can be merged with most other controllers
Arthur CHAN

Co-founder
Talkgoal.com

On Feb 18, 2015, 12:47 PM, at 12:47 PM, Richard Schneeman notifications@github.com wrote:

making it impossible to use the wick in the existing controllers. it
is very troublesome

What is your specific use case? What are you trying to do? What action
exists in the index action you're trying to inherit?


Reply to this email directly or view it on GitHub:
#165 (comment)

@schneems
Copy link
Member

This is the first time i've seen wicked breaking an inherited controller method in the ~3 years it's been around. I'm not convinced it's a use case we should be catering to. You can get around this already with metaprogramming.

Something like:

class MyController < SpreeController::Foo

  alias_method :index_without_wicked :index
  include Wicked::Wizard

  def index
    index_without_wicked
  end
end

Should get you what you need. It's too error prone to call super in the wicked index since we don't have a good idea of what that method would be doing.

@arthurtalkgoal
Copy link
Author

Hi @schneems,

Thanks for the comments,

First off, after reading your comments, my new commit didn't call the super but allow using wizard_index in wicked instead.

arthurtalkgoal@2de547c

My use case is simple make existing Registration or Configuration into stepbystep by putting decorators the include Wicked concerns; isn't it more powerful? With merely CSS , I am able to do such stepbystep. And even a existing frontend Form can be changed to stepbytep easily.

The only challenge is fixed by able to rename the index to wizard_index; which I think there is no good reason why index and show should be preserved for wicked all the time.

spree

countries

The challenge in your solution is I have no way to update Spree's or other gem's index; which is usually the cases even in private project. wicked logics works based on the fact that wicked assumed the controller are dedicated for wicked. That will limit the scaling up of wick use cases.

With this simple rename wizard_index even the

class AfterSignupController < ApplicationController
  include Wicked::Wizard

  steps :confirm_password, :confirm_profile, :find_friends

Example can be simply be using Devise Session and Regitration Controller directly. With little CSS update, the Devise form will be turned into a StepByStep quickly.

Currently way just won't work without own partials to handle the forms.

@schneems schneems closed this Sep 2, 2016
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