[WebProfilerBundle] Fix the icon for the Cache panel #23160
Conversation
javiereguiluz
commented
Jun 13, 2017
| Q | A |
|---|---|
| Branch? | 3.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #23159 |
| License | MIT |
| Doc PR | - |
|
hm i would add more attributes <svg version="1.1" xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="24" height="24" viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve">i think the other should be added to the cache icon too. |
|
@javiereguiluz would it be worth writing a simple PHPUnit test enforcing the presence of these attributes in all SVG files of the icon folder ? It would forbid reintroducing the same bug each time we add a new panel. |
|
This PR fixes the issue for me. Thanks a lot. :-) |
|
|
||
| namespace Symfony\Bundle\WebProfilerBundle\Tests\Resources; | ||
|
|
||
| use Symfony\Bundle\WebProfilerBundle\Tests\TestCase; |
There was a problem hiding this comment.
use the PHPUnit class directly instead IMO. this base testcase is useless as it is empty
| { | ||
| public function testIconFileContents() | ||
| { | ||
| $iconFiles = glob(__DIR__.'/../../Resources/views/Icon/*.svg'); |
There was a problem hiding this comment.
I suggest moving this glob to a data provider instead, so that all icon files are checked when running tests, instead of reporting only the first failed file
|
@javiereguiluz this PR should go in 3.3. Please change it to have a working CI (the master branch is currently broken) |
| */ | ||
| public function testIconFileContents($iconFilePath) | ||
| { | ||
| $this->assertRegExp('/<svg version="1.1" xmlns="http:\/\/www.w3.org\/2000\/svg" x="0px" y="0px" width="\d+" height="\d+" viewBox="0 0 \d+ \d+" enable-background="new 0 0 \d+ \d+" xml:space="preserve">.*<\/svg>/s', file_get_contents($iconFilePath), sprintf('The SVG metadata of the %s icon is different than expected (use the same as the other icons).', $iconFilePath)); |
There was a problem hiding this comment.
what about using a different delimiter ({..} or my favorite ~...~ :)) so we dont need to escape / which occurs a few times :)
There was a problem hiding this comment.
hey wouldn't a assertStringStartsWith be easier at this place?
i mean we only want to check the head of the file
There was a problem hiding this comment.
if we test this, lets test for a closing </svg> as well :) like currently. It doesnt hurt.
There was a problem hiding this comment.
@Hanmac yes, that would be easier and I used that at first ... but some icons have different sizes, so we can't use it :(
There was a problem hiding this comment.
@javiereguiluz: okay now i understand why you use regexp.
hm but if we/you guys want to do that you could check if the width and hight does correspond to the viewBox
i was just thinking that we only need to check the head of the svg
hm should we also test if the svg is valid xml?
PS: we could also add xml declaration like this
<?xml version="1.0" encoding="utf-8"?>or can't we do that because its embedded?
There was a problem hiding this comment.
We don't have the XML header in the icons and they look OK, so I guess it's optional to add it.
|
I've made the requested changes. |
|
Thank you @javiereguiluz. |
…ereguiluz) This PR was squashed before being merged into the 3.3 branch (closes #23160). Discussion ---------- [WebProfilerBundle] Fix the icon for the Cache panel | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23159 | License | MIT | Doc PR | - Commits ------- 50c1d47 [WebProfilerBundle] Fix the icon for the Cache panel
|
@javiereguiluz i think the xml Decl doesn't work because the svg are embedded? (don't know if my firefox does show it correct) |