-
Notifications
You must be signed in to change notification settings - Fork 67
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
add avatars functionality #351
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!! Thank you so much for doing this. I've got a bunch of suggestions but this is 95% there. I can help with anything if you need it.
def run(opts) | ||
warn_if_unexpected opts | ||
Dir.chdir(deck.img_dir) do | ||
defaults = { library: 'avataaars' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults = { library: 'avataaars' } | |
defaults = { library: 'avataaars', seed: 'squibrocks' } |
I like to have everything have a default so things work even if you forget something. This seemed most sensible
@@ -36,6 +36,7 @@ Gem::Specification.new do |spec| | |||
spec.add_runtime_dependency 'gio2', '~> 3.4' | |||
spec.add_runtime_dependency 'gobject-introspection', '~> 3.4' | |||
spec.add_runtime_dependency 'highline', '2.0.3' | |||
spec.add_runtime_dependency 'mechanize', '~> 2.7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mechanize might be overkill for us - I think we can download a file with Ruby's stdlib?
seed = options[:seed] | ||
next if seed.nil? | ||
|
||
file = "avatar-#{library}-#{seed}.svg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should give people control over this, similar to prefix
and suffix
. Use the % on strings to format and make that a parameter (https://github.com/andymeneely/squib/blob/dev/lib/squib/args/save_batch.rb#L56-L58).
Something like an option called filename
that defaults to `"avatar-%s-%s.svg". Like this:
irb(main):001:0> "avatar-%s-%s.svg" % ['a','b']
=> "avatar-a-b.svg"
We could also document a sample that allows you to put the avatar it is own folder:
irb(main):002:0> "avatars/%s-%s.svg" % ['a','b']
=> "avatars/a-b.svg"
string to use as the randomizer for the avatar library. Or, in the case of ``'initials'``, the actual initials shown in the returned image. | ||
|
||
width | ||
default: ``native`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default: ``native`` | |
default: ``:native`` |
avatar | ||
====== | ||
|
||
Renders an avatar SVG image using using https://avatars.dicebear.com/. Uses the SVG-specified units and DPI to determine the pixel width and height. If neither data nor file are specified for a given card, this method does nothing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to document that it'll save a file and only download if it's not saved already
@@ -0,0 +1,2 @@ | |||
avatar-*.svg | |||
avatars/*.svg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this second line? Would be nice if we had some samples that had already downloaded an avatar, that would be nice for regression testing reasons.
|
||
file = "avatar-#{library}-#{seed}.svg" | ||
|
||
# Check if we need to download the image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like this chunk to be extracted out to a separate file - that way this class is only doing DSL method delegation stuff and we can test the dicebear thing separately. Maybe in a file like lib/squib/avatar_downloader.rb
or something.
|
||
# Check if we need to download the image | ||
unless File.exist?(file) | ||
agent = Mechanize.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be good to do a log statement whenever we download. It'll be verbose the first time, but at least people would know when it's downloading and not.
draw_graph_paper width, height | ||
|
||
sample "Avatar library - 'male'." do |x, y| | ||
avatar library: 'male', seed: 'abcde', x: x, y: y, width: 96, height: 96 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an example showing different avatars on different cards?
options = defaults.merge opts | ||
|
||
# Extract the default svg options | ||
range = Args.extract_range opts, deck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as it currently stands library
and seed
won't expand per-card. I'd be fine with a new args class args/avatar_special.rb
or args/dicebear.rb
or something that has library
and seed
.
It also allows us to specify all these: https://avatars.dicebear.com/docs/options.
Oh and don't worry about the CI failure - macos + Ruby3 has been having issues on GHActions lately |
Motivation
Adding functionality use placeholder avatars.
Have you...
_samples
?docs/
folder?The above things are not required, but appreciated.