open-austin / pet-finder

An application to quickly and easily help owners find lost pets.
7 stars 5 forks source link

fixed cursor pointer over subscribe and unsubscribe forms #56

Closed tallys closed 9 years ago

tallys commented 9 years ago

fixed issue 54

tshelburne commented 9 years ago

@Tallys To start, it's good that you found a way to fix the issue, but unfortunately it’s not the right fix. However, the issue you're fixing is actually a symptom of a few bigger problems; (1) your layout file is extending from a class that is more specific than it is (.pet-icon from the main page; I honestly don’t even understand how this is compiling), and (2) there's no namespacing in your page styles.

  1. Layout styles should only include / extend styles that were defined before them, are more general than they are, and are semantic to the purpose. Extending styles that are more specific and defined later is reversing the hierarchy - you will quickly end up with a soupy mess. It already took me a while to find how this was actually happening.
  2. Your page styles should really only be able to apply to elements on that page. In other words, the classes in your page stylesheets should be nested under a class that uniquely identifies that page. In "vanilla" css, it would look something like this:

    body.main-page .pet-icon {…}
    
    body.results-page .subscription-form {…}

    Without that specificity, you are defining global styles in a place where we would expect styles local only to a certain page.

To do:

  1. Layout styles can’t depend on page styles - I missed this in my earlier code review. That needs to be pulled out. It may be a good opportunity to pull a more general style out of the .pet-icon class and put it into global - that way you can include it in both classes without having hierarchical issues. This will make the changes you created in the PR unnecessary.
  2. Let’s talk about Rails middleware, and set up an object that will have page-level effects for the layout. We discussed it before re: the header - now it’s reared it’s head again. Time to do it. Basically, we are going to use Rails middleware to create an object that will look at the incoming request and map it to a body class that we can include on body element in the layout.
  3. Once you’ve done the above, you can put all your page styles into properly nested chunks. Please do that.

Extra credit

Let's clean up the _main.scss file in general with a placeholder selector (turn .pet-icon into %pet-icon) and a mixin. Each of the animal icons include the mixin - I’d like the final product to look like this:

%pet-icon { … }
@mixin pet-icon { … }

.dog-icon { @include pet-icon(‘dog’); }
.cat-icon { @include pet-icon(‘cat’); }
.other-icon { @include pet-icon(‘other’); }

To handle the variable image names nicely, you will need to know about variable interpolation.