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 querystring to options; add JetBrains project dir to .gitignore; #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

azurelogic
Copy link

I noticed that there was no way to increase the requested image resolution without weird hacks in the template. Here's a way to apply a query string to all of the Gravatars by string or by object.

@stevenschobert
Copy link
Owner

Nice @azurelogic! Thanks for the awesome addition 👍 🍰

Adding the sizing options was always on my to-do list, just never got around to it. I've just been doing it in the templates.

With these changes, can sizes be controlled on a per-entry basis? Or are they set globally?

@azurelogic
Copy link
Author

It's global. I considered a per entry basis, but I thought the config could get weird.

This:

metalsmith.use(gravatar({
  stevenschobert: "spschobert@gmail.com"
}));

would become this:

metalsmith.use(gravatar({
  stevenschobert: {
    email: "spschobert@gmail.com"
    querystring: {...}
  }
}));

It's not so bad with one or two, but it could look a little crazy if you did it for a bunch of them in various groupings. I suppose the global setting could be overridden by individual settings. I'll see what I can do with that this evening.

@stevenschobert
Copy link
Owner

Yeah I can see it would get a little busy with multiple entries. Though we could make it so that you can supply either a string or an object.

metalsmith.use(gravatar({
  stevenschobert: {
    email: "spschobert@gmail.com"
    size: {...}
  },
  testuser: "test@test.com"
}));

This would make the changes backwards compatible, as well as let people opt-in to controlling options on specific entires.


Something else that you got me thinking about; should we use the option names like size rating default, rather than exposing querystring as an option?

metalsmith.use(gravatar({
  stevenschobert: {
    email: "spschobert@gmail.com"
    size: 200,
    rating: "g",
    default: "http://myapp.com/default_avatar.png"
  },
  testuser: "test@test.com"
}));

I'm wondering if that might be a little more user friendly than exposing querystring? This means the user won't have to look up the options Gravatar supplies, and figure out how to build the querystring themselves. It would also give us a place to put additional logic around individual options later, without breaking the API.

What do you think? Does gravatar supply too many options that would make that difficult to implement?


Also, you are awesome for helping, and I really appreciate it! If you don't have to time to fuss with any of that I totally understand!

@azurelogic
Copy link
Author

I'm inclined against the granular options only because it allows for Gravatar to add (or even change) their API without requiring someone any updates to the mappings in this plugin. I figured that this configuration option was a good compromise:

{
  "plugins": {
    "metalsmith-gravatar": {
      "options": {
        "querystring": {
          "s": 200,
          "r": "pg"
        }
      },
      "avatars": {
        "stevenschobert": "spschobert@gmail.com",
        "authors": { ... }
      }
    }
  }
}

As for the individual settings, I'm thinking of adding them in such a way that they would just override the global settings. Sound good?

@azurelogic
Copy link
Author

I've got something working. It's not quite as elegant as before, because we need to detect whether the user id is being set to an email address string or an object in the middle of mutateStringsInObject(). I need to finish the documentation, then I'll can either squash everything into a single pull request, or you can merge this one, and I'll make a separate PR. Let me know which you prefer, and I'll get it wrapped up in the next few days.

@stevenschobert
Copy link
Owner

I see your point about flexibility and the gravatar api. Though I don't see gravatar changing their api in the near future, so I'm not convinced that flexibility is really necessary. Really I was thinking more about "who uses metalsmith? do they know what a querystring is?".

As for the pull-request, you can keep all the commits in this PR and squash, so we see all the changes and conversation in the same place. :)

@azurelogic
Copy link
Author

Sure. I guess my thought was that metalsmith users have to have some knowledge of node, npm, and front-end dev. I would hope that most of them to know what a query string is. In addition, I added a link to Gravatar's docs from the documentation. If you think it adds value, we can add custom mapping, but I felt it would make the code less elegant and more brute force.

I'm currently prepping a talk to give tomorrow that was sprung on me last minute. I should be back to this in the next few days though.

@azurelogic
Copy link
Author

Alright, I have everything together for individual overrides, including tests and documentation. I think it works pretty well this way, and it's more hands off in the case of future additions/changes. If you still think that it still needs more explicit query string params though, let me know. We would have to figure out whether we want to allow both "s" and "size" or just "size".

@azurelogic
Copy link
Author

@stevenschobert What are your thoughts on merging this in and deploying it on npm? Do you think we need the additional mapping, or do you want to proceed from here?

@azurelogic
Copy link
Author

Just saw this in my list of open PRs and thought I would follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants