search

jasonlong / geo_pattern

Create beautiful generative geometric background images from a string.
http://jasonlong.github.io/geo_pattern/
MIT License
1.25k stars 88 forks source link

Add background color accessor #45

Closed jarthod closed 9 years ago

jarthod commented 9 years ago

I saw this on the js version and it seemed really usefull to me for fallback or styling other element. I was sad to see it wasn't in the ruby version :cry: so here it is!

jasonlong commented 9 years ago

:thumbsup: Makes sense to me.

@maxmeyer @ttaylorr Is this safe to merge now or should we wait for the current structural changes?

maxmeyer commented 9 years ago

No, it's not safe to merge. It interferes with #40 and #44. It would be best to integrate it with #44. But I like the idea. We should add that functionality. Something like. Furthermore the style of tests will change a lot if @ttaylorr goes with the style introduced in the latest commits of #40.

class Pattern
  attr_reader :color
end
jarthod commented 9 years ago

If you're planning on merging #40 soon I'll happily rebase this with the new code style ;)

jasonlong commented 9 years ago

Ok, #40 is merged.

maxmeyer commented 9 years ago

@jarthod Maybe you want to wait a bit. I'm going to refactor the generators and will add a Pattern-class. This could hold the value you're interested in.

jarthod commented 9 years ago

Ok, I was currently looking at the rebase and saw that with the new code I can only easily get the "rgb()" value, which is enough for the usecase I see (fallback for browser) but if we can make this more generic it's even better ;)

BTW is there any reason for the private attr_reader thing ? is it convention ? it looks like instance variable would be fine to me.

jarthod commented 9 years ago

I temporarily pushed my changes to the branch. Let me know when I shall rebase again ;)

maxmeyer commented 9 years ago

BTW is there any reason for the private attr_reader thing ? is it convention ? it looks like instance variable would be fine to me.

This makes refactoring a lot of easier. You just need to change the method implementation instead of changing all places where you use the instance variable.

And as the attr*-thing should be not part of the public api of the object I marked them as private.

jarthod commented 9 years ago

The point of the feature is to expose the color attribute ;) if you make it private is just won't work. BTW I think you pushed back the un-rebased branch of my work (at least the spec and readme). Maybe I should have to a regular merge, but I just wanted to keep the commits clean ;)

maxmeyer commented 9 years ago

I think I will expose it in the newly created Pattern-class.

jarthod commented 9 years ago

Ok, let me know if you need any help/review then ;)

maxmeyer commented 9 years ago

I will do. :smile:

ttaylorr commented 9 years ago

Yeah, this needs to wait for @maxmeyer's changes, but after that, I'm :+1: with this.

maxmeyer commented 9 years ago

@jarthod

Can you review the current version on master if the new api is helpful for you? See https://github.com/jasonlong/geo_pattern#inspection-of-pattern for a more detailed description on the new api.

Cheers

maxmeyer commented 9 years ago

Are there any other features found on the js version which might be helpful for you as well in the ruby version?

jarthod commented 9 years ago

This looks great :+1: It does more than the js version now. This will be 1.4 I guess ?