SVGO location

[fragkakis]

Hello,

Nice library!

Currently SVGO needs to be installed globally in order to be used. The change I made makes use of the svgo parameter, which can now be used to specify the path where svgo is found. The SVGO to be used is chosen in order of precedence:

1. The specified SVGO parameter, if it is an absolute or relative path
2. The SVGO under ./node_modules/svgo/bin
3. The globally installed SVGO (current)

I have added (locally) the following tests, but did not commit them as they require svgo.

  describe '#build_destination_file_contents_with_svgo_set_to_true' do
    let :sprite_builder do
      Svgeez::SpriteBuilder.new(
        'source' => './spec/fixtures/icons',
        'destination' => './spec/fixtures/icons.svg',
        'svgo' => 'true',
      )
    end

    it 'returns a string representation of combined SVG files.' do
      expect(sprite_builder.send(:build_destination_file_contents)).to eq IO.read('./spec/fixtures/icons_svgo.svg')
    end
  end

  describe '#build_destination_file_contents_with_svgo_absolute_path' do
    let :sprite_builder do
      Svgeez::SpriteBuilder.new(
        'source' => './spec/fixtures/icons',
        'destination' => './spec/fixtures/icons.svg',
        'svgo' => '/Users/fragkakis/Documents/workable/node_modules/svgo/bin',
      )
    end

    it 'returns a string representation of combined SVG files.' do
      expect(sprite_builder.send(:build_destination_file_contents)).to eq IO.read('./spec/fixtures/icons_svgo.svg')
    end
  end

  describe '#build_destination_file_contents_with_svgo_relative_path' do
    let :sprite_builder do
      Svgeez::SpriteBuilder.new(
        'source' => './spec/fixtures/icons',
        'destination' => './spec/fixtures/icons.svg',
        'svgo' => '../workable/node_modules/svgo/bin',
      )
    end

    it 'returns a string representation of combined SVG files.' do
      expect(sprite_builder.send(:build_destination_file_contents)).to eq IO.read('./spec/fixtures/icons_svgo.svg')
    end
  end

[jgarber623]

Thanks for the PR, @fragkakis! I'm in the midst of some travel, but will review your changes as soon as possible.

[jgarber623]

@fragkakis So that I can more easily review your code locally, could you implement these changes in a branch other than master? That may or may not require a new PR. Thanks!

[jgarber623]

Actually… I think we could avoid a lot of SVGO-related code by using the svgo_wrapper gem. It includes a check for SVGO's presence that might handle cases where SVGO is installed at the project level in ./node_modules.

What do you think? I can create a branch with the svgo_wrapper and give it a shot.

[fragkakis]

Sounds good!

On Oct 9, 2016 17:35, "Jason Garber" notifications@github.com wrote:

Actually… I think we could avoid a lot of SVGO-related code by using the
svgo_wrapper https://github.com/tribune/svgo_wrapper gem. It includes a
check for SVGO's presence
https://github.com/tribune/svgo_wrapper/blob/master/lib/svgo_wrapper/svgo_check.rb
that might handle cases where SVGO is installed at the project level in
./node_modules.

What do you think? I can create a branch with the svgo_wrapper and give it
a shot.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#21 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AANmm065mSvgF2UrRR-yjSu1a6S1X7vQks5qyPu5gaJpZM4J-4It
.

[jgarber623]

Implemented svgo_wrapper locally and found one thing I'm not fond of: https://github.com/tribune/svgo_wrapper/blob/master/lib/svgo_wrapper/svgo_check.rb#L20-L29

The gem checks for SVGO when its required rather than when it's initialized with SvgoWrapper.new, ostensibly for performance reasons. That would make SVGO a "requirement" in order to silence warnings.

🤔

I'm gonna do a bit more thinking on this…

[diniscorreia]

Looking to use a local version of SVGO instead as well. Do you still plan to go ahead with this?