search

sul-dlss / sul_styles

A gem for adding Stanford University Libraries styles to Rails applications
Other
1 stars 1 forks source link

Add links to headings on Colors page #24

Closed ggeisler closed 8 years ago

ggeisler commented 9 years ago

Very minor, but I thought it would be useful for the references on the color page be clickable links:

stanford_university_libraries_-_style_guide 2

I did this in a simple way so feel free to reject or do something different. With this commit, the page looks like this:

stanford_university_libraries_-_style_guide

mjgiarlo commented 9 years ago

:+1:

jkeck commented 9 years ago

I wonder if it makes sense to do this splitting in the the SULStyles::Colors class ( https://github.com/sul-dlss/sul_styles/blob/master/lib/sul_styles/colors.rb#L25 ) and have title and link or something (which can be more easily tested).

Up to you though, I'm fine with :ship:ing as-is.

mjgiarlo commented 9 years ago

Makes sense to me to have instances of that class have a title/label and a link vs. doing string manipulation in a template. Or to encapsulate the splitting into a helper.

ggeisler commented 9 years ago

Yeah, as soon as I submitted the pull request it occurred to me that I should've put the logic in a helper. (I think I was being lazy because a helpers directory doesn't yet exist.) I'll take a look soon and fix up using both of your suggestions and update the pull request.

jkeck commented 9 years ago

:eyes: @ https://github.com/sul-dlss/sul_styles/blob/master/lib/sul_styles/colors.rb#L25

mejackreed commented 9 years ago

:ship: it?

ggeisler commented 9 years ago

@mejackreed Up to you. I was planning on updating the pull request to take into account the suggestions above from @jkeck and @mjgiarlo. It's a small fix but not high on my priority list right now.

jkeck commented 9 years ago

@ggeisler would you accept a PR to this? I'm more than happy to do it.

ggeisler commented 9 years ago

Sure @jkeck! Thanks.