Icon objects

[natewiebe13]

Here's an implementation of #19 (also implements #12 and #33). I usually try to keep a lot of changes separate to make review easier, but sometimes it's necessary to make sure it feels cohesive together.

There's a few benefits we get here. First is the API is basically the same as before. You can do the following.

<?php $icons = new \Feather\IconManager(); ?>

<button type="button"><?= $icons->get('trash'); ?></button>

get() will now return an Icon object though. Which means that you could get the icon anywhere and be able to manipulate it's attributes up until you decide to render it. This would also work great for serialization so you could store the icon in a message queue or something and then deal with it later on and modify the size, color, etc if you need to before you actually use it.

So this gives us the simplicity of what existed before, but allows more advanced usages as well.

A couple things you might have questions about:

• Attributes with null values are now filtered out. This will be exceptionally useful when adding the Symfony bundle so we can null an attribute instead of having to have a separate control mechanism for removing.
• The attribute test will now also check that we aren't double escaping when we escape attribute values.
• I'm currently using IconManager I was also contemplating IconRegistry. Let me know if you prefer the latter or something else.

Let me know what you think.

[Pixelrobin]

I like this, it makes sense to separate an icon from a manager. I'm fine with IconManager personally but I think both names would work. The __toString() conversion is clever, I like it.

Good call on the null attribute handling and boolean attributes. I'm not 100% sure what you mean by "adding the Symfony bundle" never mind, you mean when this project becomes a symphony bundle.

Some thoughts about the API, and with #33 in mind. What if instead of setAttributes and setAttribute was just attributes and attribute? So in the future you could do (as an extreme example):

<?= $icons->get('anchor')->color('red')->strokeWidth(3)->attribute('data-label', 'anchor')->attributes(['foo' => 'bar']);  ?>

(Or put the helper functions and the attribute changes before the get in the chain to make them global to the manager)
It's more of a stylistic choice, but I feel like putting set on everything there would be cumbersome. Thoughts?

For completion's sake, I think there should be additional tests for for the API. Testing things like setting individual attributes and also all the nuances of how attributes are treated (for example null). But, the tests need an overhaul in spots anyways so maybe that can be a later change.

[natewiebe13]

Just to clear things up regarding the Symfony bundle and Drupal module I've mentioned previously. Best practice is to keep them as separate repos from a generic php library. So I think it makes sense to keep this framework agnostic, but still do things in consideration of what framework glue packages would benefit from, provided it doesn't make the quality of this library worse.

Regarding whether to add set/get prefixes to functions. Personally I find including them to be valuable. We will likely be very familiar with the API so if we did drop a prefix, we'd know what the function does without having to dig into docs/source code. But I think the average dev would need to dig in a bit more to know what functions do. By adding a get/set prefix, it's pretty obvious what's going on. I've also been thinking about proposing getIcon() instead of just get(). At first glance, it might seem like it's cumbersome, but since our IDEs have autocompletion, I don't think it's a real concern. It also help if you start typing icon since you want to get an icon, and have getIcon() show up, where just get() by itself wouldn't. get() also doesn't really describe what you're getting without looking at the function definition, but that's another topic. I would also want to stay away from a single function that can get and set as that is not best practice.

Looking at your extreme example, I think we should have a mechanism for being able to set a bunch of attributes at once that feels like the current get() function, probably by just adding that to the render() function as well.

Regardless, I think for next steps, I'll work on the API and write some templates and scenarios and see what feels good and what doesn't and once it feels right, I'll assign back for another review!

[natewiebe13]

I've rethought about adding attribute modification to the render function, but I don't think it helps in this context, and then it basically acts similarly to just calling setAttributes(), which would then render afterwards if __toString() in invoked. So we don't gain anything other than code smell.

I've also moved attribute handling into a trait as that makes sense in our case here.

@Pixelrobin I'm curious to get your thoughts on the current state. Ignore the failing tests for now, we'll fix them up once we land on an API to try out. I'd like to focus on getting something close, rather than 100%. I'd like to release an alpha version soon and try this out on some real projects and get some feedback as we might not see something until we go to use it.

[Pixelrobin]

Explicit get and set prefixes is fine. It's a stylistic choice at the end of the day. If that's something you're more used to, then lets do that.