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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feather dependency #6

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@craigrodway
Copy link
Contributor

commented Jun 19, 2019

Hi! 馃憢

I've been thinking about #3 and ways this could be done - or achieve a similar result.

Not sure if you're aware of https://asset-packagist.org/, but essentially it allows PHP projects to pull in packages from Bower or npm registries as if they were regular PHP packages. If php-feather is updated to require the npm package for feather-icons, it should remove the need to re-build this package on every Feather update.

"require": {
	"php": ">=5.3.0",
	"npm-asset/feather-icons": ">=4"
},

This means projects that require php-feather also require npm-asset/feather-icons - which means they can use the most recently published version which this library will then use (instead of the bundled ones). This does impact the way the icons are loaded, how it is tested, and no longer requires building. But it seems to work.

This PR includes the following changes:

  • Update Icons class to load the icons.json file from the npm package. This is only 52kb at the moment - so I think any performance/memory concerns of the single JSON file loaded vs loading each file on-demand separately isn't anything to be worried about. This sort of resolves #5 too!
  • As php-feather isn't re-built, the default attributes are embedded directly into the class.

This also essentially removes the requirement on NodeJS for development/building. I haven't removed those yet, but don't mind doing that and some tidying-up if you're happy with the changes and direction. Look forward to hearing your thoughts!

@craigrodway

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

Just coming back to this, have just realised this only works when the project requiring pixelrobin/php-feather also includes this in the composer.json:

"repositories": [
    {
        "type": "composer",
        "url": "https://asset-packagist.org"
    }
]

Back to the drawing board, perhaps? 馃檨 馃檪

@Pixelrobin

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2019

Hey @craigrodway, thanks for the PR/suggestion! Sorry for the late response, I'm a bit inactive on GitHub at the moment. I don't have time at the moment to look into this at depth right now, but I'll try to within a few days.

@craigrodway

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

Hey! No problem at all :) That's fine, no rush. I'm still not sure if it's the best way to do it, and am still looking at alternatives :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.