Navigation Menu

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

colorize toolbar label if one or more blocks were rendered #231

Closed
wants to merge 1 commit into from
Closed

colorize toolbar label if one or more blocks were rendered #231

wants to merge 1 commit into from

Conversation

mvhirsch
Copy link
Contributor

@OskarStark As mentioned in #229.

@soullivaneuh
Copy link
Member

Could we have a before/after screenshot?

Please take a look to SF 2.8 toolbar too.

@OskarStark
Copy link
Member

bildschirmfoto 2015-10-14 um 16 05 04
bildschirmfoto 2015-10-14 um 16 05 08

before it was grey 😄

@soullivaneuh i would like to merge this PR as it is.

IMO we have to check and open a separate issue/PR for adjustments for 2.8 toolbar

Thank you @mvhirsch

@OskarStark OskarStark self-assigned this Oct 14, 2015
@soullivaneuh
Copy link
Member

IMO we have to check and open a separate issue/PR for adjustments for 2.8 toolbar

No. 2.8 compatibility is already here. We can't accept broken PR for that. Both need to be handled.

@OskarStark
Copy link
Member

Ok, but i don't understand how adding a class name to a span could broke the panel...

can you explain more?

@soullivaneuh
Copy link
Member

See this line: https://github.com/sonata-project/SonataBlockBundle/pull/231/files#diff-8d1d2fb5d440bda24d37396fb7790e89R7

Behavior should be the same. So we have to check this.

@OskarStark
Copy link
Member

good catch @soullivaneuh

@mvhirsch can you please also handle the 2.8 version?

@mvhirsch
Copy link
Contributor Author

@OskarStark @soullivaneuh I don't think we need to colorize in the new symfony toolbar.
We should only colorize warnings/errors according to this blog entry:

http://symfony.com/blog/new-in-symfony-2-8-redesigned-web-debug-toolbar

[...] Another important difference is the way errors and warnings are displayed. Previously we displayed just a small green/yellow/red icon, whereas now we change the entire panel background to easily spot when some error happens [...]

@mvhirsch
Copy link
Contributor Author

What do you think @OskarStark @soullivaneuh?

@OskarStark
Copy link
Member

👍 @mvhirsch

@soullivaneuh
Copy link
Member

I don't know if we really should colorize it. See doctrine request, no colors.

Green color seems to be used for successful stuff like ajax request and more.

Not sure if this should be applied here.

ping @rande @sonata-project/contributors

@soullivaneuh
Copy link
Member

According to the new Sonata version management and next major release plan, this project has been refactored regarding branching and versioning.

If you see this message, your PR concerns a patch or a minor release and is not targeting the right branch.

So I'm closing this one, but don't see it as a refusal. If you think your work is still relevant and want to continue, feel free to reopen it on the right branch (e.g. the default one).

Regards.

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

Successfully merging this pull request may close these issues.

None yet

3 participants