search

dnadesign / silverstripe-elemental-virtual

Allows Content Blocks to be reused between pages.
BSD 3-Clause "New" or "Revised" License
7 stars 26 forks source link

use link-icon from silverstripe font-icons #32

Closed lerni closed 3 years ago

lerni commented 4 years ago

fixes color & background (new transparent) and also alignment, removes custom svg-icon

before: before-custom-icon

after: after-ss-icon

Fixes: #33

sachajudd commented 4 years ago

The builds are fixed in the latest master branch, can you rebase to get this PR green?

robbieaverill commented 4 years ago

Hi @lerni, your PR looks great and makes the module better. Unfortunately we need to consider backwards compatibility, and this module doesn't have a minimum SilverStripe core version it requires. This means that people on SilverStripe 4.0 could be using it, and they would get a broken link from this font which I believe was introduced in SilverStripe 4.3 or 4.4 or so.

In order to retain backwards compatibility, you can either leave the CSS, image, and composer.json change in place and only change the icon config, or a safer/better option would be to add a composer dependency for silverstripe/admin at the version the icon was introduced in. I don't know what that was, but I'd say ^1.4 should be safe enough.

sachajudd commented 3 years ago

@bergice do you think you could take a look at the failing tests?

bergice commented 3 years ago

@bergice do you think you could take a look at the failing tests?

@sachajudd, the failure is due to the version conflicts introduced by adding the admin dependency in this PR.

Also, note that this module is not part of our supported modules list.

dnsl48 commented 3 years ago

@bergice I've made a patch for CI - https://github.com/dnadesign/silverstripe-elemental-virtual/pull/36

dnsl48 commented 3 years ago

@lerni sorry, I've squashed the PR, but accidentally pushed master into your repo, so GitHub lost the PR. The work isn't lost, I've recreated it here - https://github.com/dnadesign/silverstripe-elemental-virtual/pull/37 (you're still the author of the commit)

dnsl48 commented 3 years ago

Merged in https://github.com/dnadesign/silverstripe-elemental-virtual/pull/37

lerni commented 3 years ago

@dnsl48 thank you - crazy git stuff changing an icon ;)