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

Class changes for "search", "flexible search", "search level", & "selection" #18

Open
joshribakoff opened this issue Oct 19, 2013 · 1 comment

Comments

@joshribakoff
Copy link
Member

These class names are confusing. I'll be the first to admit the way I did this is crap. It can be much better. This ticket will discuss how to make it better

Old Way
Search - is an analogue for the search template's Magento block. The "block" is a useless Magento concept, and is an excuse to put logic into the view layer.

Search Level - This class renders a selectbox's HTML. It also calculates which option is selected & adds the "selected" html attribute to the right tag.

Flexible Search - This class sort of represents what the user's current selection is.

Selection - this is like a collection class, which lists many vehicle objects.


New Way
There should be two classes. The search form, and the selection

Search Form - Renders all HTML for displaying the search form/drop-down boxes.

Selection - Represents the parameters that is the current selection. Unlike the old way where its just a wrapper around an array of vehicles, this will just show the parameters, so its actually just a wrapper around $_GET / $_SESSION. In order to see which vehicles actually match this selection, you have to pass the selection class to a finder class

@joshribakoff
Copy link
Member Author

So I've tried to fix this incrementally a few times on some local branches. I had problems where changes would break tests, and I would spend way too long trying to get the tests to pass. I kept trying to reset --hard & redo it in smaller steps, unsuccessfully. I think it may make sense to create a branch, and delete the 4 classes, and build the two new ones from scratch (as opposed to trying to refactor to the two new classes).

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

No branches or pull requests

1 participant