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.

Sign up for GitHub

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

Jump to bottom

removed famfamfam icon set #4315

Merged
merged 1 commit into from
Feb 22, 2017
Merged

removed famfamfam icon set #4315

merged 1 commit into from
Feb 22, 2017

Conversation

OskarStark
Copy link
Member

@OskarStark OskarStark commented Feb 8, 2017 •
edited
Loading

Closes #4314

Changelog

### Removed
- famfamfam icon set

@greg0ire greg0ire added the patch label Feb 8, 2017
@OskarStark
Copy link
Member Author

😄
screenshot 2017-02-08 11 05 36

@jvasseur
Copy link
Contributor

jvasseur commented Feb 8, 2017

This should probably go to master, it is kind of a BC break if someone was relying on sonata have those icons available.

@OskarStark
Copy link
Member Author

This should probably go to master, it is kind of a BC break if someone was relying on sonata have those icons available.

Yeah, this is my thought, too

@greg0ire
Copy link
Contributor

greg0ire commented Feb 8, 2017

Let's play it safe then.

@greg0ire greg0ire added major and removed patch labels Feb 8, 2017
@OskarStark OskarStark changed the base branch from 3.x to master February 8, 2017 10:08
@SonataCI
Copy link
Collaborator

SonataCI commented Feb 8, 2017

Could you please rebase your PR and fix merge conflicts?

@OskarStark OskarStark changed the title removed famfam icon set removed famfamfam icon set Feb 8, 2017
@OskarStark
Copy link
Member Author

Updated and now against master

greg0ire
greg0ire requested changes Feb 8, 2017
View reviewed changes
Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an UPGRADE note

@OskarStark
Copy link
Member Author

Please add an UPGRADE note

Done, are you fine with it?

greg0ire
greg0ire reviewed Feb 8, 2017
View reviewed changes
UPGRADE-4.0.md Outdated
@@ -1,6 +1,10 @@
UPGRADE FROM 3.x to 4.0
=======================

## Removed `famfamfam` icon set

If you still need it, please add it by your own!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"please set it up it on your own" or, "please set it up yourself"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@OskarStark
Copy link
Member Author

ping @greg0ire

greg0ire
greg0ire requested changes Feb 8, 2017
View reviewed changes
UPGRADE-4.0.md Outdated
@@ -1,6 +1,10 @@
UPGRADE FROM 3.x to 4.0
=======================

## Removed `famfamfam` icon set

If you still need it, please set it up by your own!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"on your own"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@core23
Copy link
Member

core23 commented Feb 8, 2017

Do we still use this in any template?

@OskarStark
Copy link
Member Author

Do we still use this in any template?

TO be honest, not sure how to test this...

@core23
Copy link
Member

core23 commented Feb 8, 2017

Don't know the famfamfam icons set, but there should be some references like class="fa fa-list" or anything else.

@OskarStark
Copy link
Member Author

I don't think so, there is no css which belongs to them

@OskarStark
Copy link
Member Author

Imo we should merge this, as it is in masterand not released yet, so if we found some problems afterwards we could patch them.

Or a bash guru can get all file names of the icons and grep through all sonata bundles...

@mvhirsch
Copy link
Contributor

Bash guru here, just did a short grep of "famfamfam" in one of my projects which uses nearly all sonata-bundles. Found matches:

$> grep -R famfamfam sonata-project/

sonata-project/admin-bundle/Resources/meta/LICENSE:http://www.famfamfam.com/lab/icons/silk/
sonata-project/page-bundle/Resources/views/PageAdmin/select_site.html.twig:                        <img src="{{ asset('bundles/sonataadmin/famfamfam/accept.png') }}">

The first match can be ignored (this MR removes it anyway). The second one is in PageBundle, the usage of the image should be removed too. Of course, this needs to be adressed on all bundles (nearly all != all) ... :-)

@OskarStark OskarStark mentioned this pull request Feb 21, 2017
Merged
@OskarStark
Copy link
Member Author

Thank you @mvhirsch

This PR can be merged after: sonata-project/SonataPageBundle#787

@OskarStark
Copy link
Member Author

@greg0ire can you please push the button ;-)

@greg0ire greg0ire merged commit 9c76233 into sonata-project:master Feb 22, 2017
@greg0ire
Copy link
Contributor

Sure!

@OskarStark OskarStark deleted the famfam branch February 22, 2017 07:39
@jordisala1991 jordisala1991 mentioned this pull request Mar 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

@core23 core23 core23 approved these changes

Assignees
No one assigned
Labels
major
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@OskarStark @jvasseur @greg0ire @SonataCI @core23 @mvhirsch @soullivaneuh