Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork473
Update collector template for the new toolbar design#445
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Update collector template for the new toolbar design#445
Uh oh!
There was an error while loading.Please reload this page.
Conversation
99bbf96 to32fe207Comparejaviereguiluz commentedJul 20, 2015
I'm about to update the collectors of other bundles. Therefore, it would be great if someone could validate that this strategy is valid to maintain BC. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it would be better to have a way to detect the version of the WebProfilerBundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right, but I don't know how to do that 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
a solution would be to make the WebProfilerBundle pass a variable to this template to tell it should display the Sf 3 design. If the variable is not there, the template would assume it is the old bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this could even work in the future for further design changes by changing the value of the variable btw
javiereguiluz commentedJul 20, 2015
@stof I like your last proposal. I'm trying to implement it injaviereguiluz/symfony@44f101f andjaviereguiluz@83d04a1 |
83d04a1 toa4aed77CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this will break for older versions of Symfony where the constant is not available
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I propose to revert to the previous way of detecting the toolbar version to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
which previous way ? Checking the kernel version id here ? This was broken (even though your implementation oftoolbar_version is currently broken in the same way in the Symfony PR)
javiereguiluz commentedJul 27, 2015
I've reverted all the recent changes made to this PR. Now we're back to the original situation: {%setis_symfony_2_8_or_higher=constant('Symfony\\Component\\HttpKernel\\Kernel::VERSION_ID')|default(0)>=20800 %}{%ifis_symfony_2_8_or_higher %} ...{%else %} ...{%endif %}If you release a new version of this bundle compatible with Symfony 2.3+, that code should be enough to display the appropriate toolbar depending on the Symfony version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this needs to be done inside thetoolbar block too, because the rendering of the toolbar does not render the full template but only the toolbar block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and actually, it is needed only inside the toolbar block for now, as the profiler itself does not yet have a new markup and so we don't need this variable yet there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated it. Thanks.
stof commentedJul 30, 2015
Before merging this, I will wait until the Symfony PR gets merged, to be sure that no other changes to the template are needed based on some review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we should avoid displaying a red box for people using Doctrine ORM 2.4 as they cannot enable the SCL if it does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
OK. Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you are keeping the green background here when the value is 0 while removing it for invalid entities. What is the reasonning about when a green background should be used in info pieces ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right. The green background is no longer displayed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest extracting the icon to a.svg file. This way, it is easier to reuse it for the menu panel, and Github is able to display a preview.
As we have a single icon here, it may be placed in the Collector folder rather than creating a separate folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've done this change. However, the new icon is not used yet in the profiler. The reason is that the new SVG icon has been designed for the new toolbar. We can't use it in the current profiler because the icon's style doesn't match the other icons. This will be done when we redesign the profiler and change all the icons at the same time. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah, I know that the panel is not yet redesigned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
wrong template name. The Twig namespace for the bundle is@Doctrine, not@DoctrineBundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed. Thanks.
stof commentedAug 12, 2015
Thank you@javiereguiluz. |
… (javiereguiluz)This PR was squashed before being merged into the 2.3-dev branch (closes#109).Discussion----------Update the toolbar and panel for the new profiler designThis PR adds support for the new Symfony Toolbar (seesymfony/symfony#15160) and it uses the same trick validated by@stof for the DoctrineBundle (seedoctrine/DoctrineBundle#445).Commits-------4cfc35f Update the toolbar and panel for the new profiler design
Symfony is redesigning its toolbar for 2.8+ versions (seesymfony/symfony#15160). We need to update Doctrine toolbar panel too.@stof told me to not change the current HTML markup, so that's why I include two different contents depending on the Symfony version.