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

add avatars functionality #351

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

rnewton
Copy link

@rnewton rnewton commented Nov 6, 2021

Motivation

Adding functionality use placeholder avatars.

_avatars_00

Have you...

  • Written a sample in _samples?
  • Updated documentation in the docs/ folder?
  • Linked this PR to an issue?
  • Written any automated tests?
  • Refactored your code to be maintainable?

The above things are not required, but appreciated.

Copy link
Owner

@andymeneely andymeneely left a 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' }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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'
Copy link
Owner

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"
Copy link
Owner

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``
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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.

@andymeneely
Copy link
Owner

Oh and don't worry about the CI failure - macos + Ruby3 has been having issues on GHActions lately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants