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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an icon to admin menu. #1142

Merged
merged 2 commits into from Feb 8, 2017
Merged

Add an icon to admin menu. #1142

merged 2 commits into from Feb 8, 2017

Conversation

jlamur
Copy link
Contributor

@jlamur jlamur commented Nov 13, 2016

This is my first PR, I'm sorry if I do something in the wrong way !

I am targetting this branch, because it's a minor change that does not break BC.

Changelog

### Added
- An icon to admin menu (fa-image).

Subject

There was no image in the admin menu. So I added one :)

@@ -1,6 +1,9 @@
<?xml version="1.0" encoding="UTF-8"?>
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
<parameters>
<!-- COMMON -->
<parameter key="sonata.media.admin.groupname">sonata_media</parameter>
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this have to do with the issue at hand exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing. Should I remove it? Or create another commit if it has any kind of interest for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what interest it has exactly, but if you find one, then make sure it appears in your commit message. As a rule of thumb, try to have commit messages that look like this :

What you are doing
<blank line>
Why you are doing it

More information about commit messages can be found in the CONTRIBUTING.md

Anyway, nice first PR, it is quite clean (cleaner than the average PR IMO).

@OskarStark
Copy link
Member

how do we set the icon for the other bundles? is it done in this way?

@jlamur
Copy link
Contributor Author

jlamur commented Nov 14, 2016

@OskarStark Yes. It's almost a copy/paste from SonataUserBundle.

It is also the reason why I introduced the parameter sonata.media.admin.groupname (see @greg0ire comment)... There's a parameter sonata.user.admin.groupname in the user bundle.

@greg0ire
Copy link
Contributor

It is also the reason why I introduced the parameter sonata.media.admin.groupname (see @gregoire comment)... There's a parameter sonata.user.admin.groupname in the user bundle.

Then your commit message should read

Add parameter for group name

Let's be consistent with the user bundle.

@jlamur
Copy link
Contributor Author

jlamur commented Nov 14, 2016

Thank you for theses advices @greg0ire.
I'll try to follow your recommendations, then edit my commits and this pull request when I'll have the time to.

@greg0ire
Copy link
Contributor

greg0ire commented Nov 14, 2016

You're welcome. The way I see it, the easiest way for you to do that would be :

  1. forget the commit (but not the changes) : git reset <reference to the last commit that isn't yours>
  2. prepare commits one by one : git add -p, then git commit
  3. overwrite anything that is on this github branch with your new work : git push --force

There are more complex solutions with git rebase -i but in that particular case I think you'll be faster without.

Let's be consistent with the user bundle.
@jlamur
Copy link
Contributor Author

jlamur commented Nov 15, 2016

Done :).

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.

LGTM

@jordisala1991
Copy link
Member

Is this PR waiting for something or we can merge it?

@greg0ire
Copy link
Contributor

greg0ire commented Feb 7, 2017

@sonata-project/contributors please review

@greg0ire greg0ire merged commit 6287e96 into sonata-project:3.x Feb 8, 2017
@greg0ire
Copy link
Contributor

greg0ire commented Feb 8, 2017

Thanks @jlamur !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants