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

Move old brand icons to piwik icons #11635

Merged
merged 10 commits into from May 6, 2017
Merged

Conversation

Findus23
Copy link
Member

First step towards #11626

I'll look into the tests later.

@Findus23
Copy link
Member Author

Findus23 commented Apr 19, 2017

As usual we need to change plugins/CustomDimensions. (hopefully for the last time)

diff --git a/tests/System/expected/test___Live.getLastVisitsDetails_year.xml b/tests/System/expected/test___Live.getLastVisitsDetails_year.xml
index 9b55fd8..899c81f 100644
--- a/tests/System/expected/test___Live.getLastVisitsDetails_year.xml
+++ b/tests/System/expected/test___Live.getLastVisitsDetails_year.xml
@@ -65,7 +65,7 @@
 		<languageCode>fr</languageCode>
 		<language>French</language>
 		<deviceType>Desktop</deviceType>
-		<deviceTypeIcon>plugins/DevicesDetection/images/screens/normal.png</deviceTypeIcon>
+		<deviceTypeIcon>plugins/Morpheus/icons/dist/devices/desktop.png</deviceTypeIcon>
 		<deviceBrand>Unknown</deviceBrand>
 		<deviceModel />
 		<operatingSystem>Windows XP</operatingSystem>
@@ -183,7 +183,7 @@
 		<languageCode>fr</languageCode>
 		<language>French</language>
 		<deviceType>Desktop</deviceType>
-		<deviceTypeIcon>plugins/DevicesDetection/images/screens/normal.png</deviceTypeIcon>
+		<deviceTypeIcon>plugins/Morpheus/icons/dist/devices/desktop.png</deviceTypeIcon>
 		<deviceBrand>Unknown</deviceBrand>
 		<deviceModel />
 		<operatingSystem>Windows XP</operatingSystem>
@@ -299,7 +299,7 @@
 		<languageCode>fr</languageCode>
 		<language>French</language>
 		<deviceType>Desktop</deviceType>
-		<deviceTypeIcon>plugins/DevicesDetection/images/screens/normal.png</deviceTypeIcon>
+		<deviceTypeIcon>plugins/Morpheus/icons/dist/devices/desktop.png</deviceTypeIcon>
 		<deviceBrand>Unknown</deviceBrand>
 		<deviceModel />
 		<operatingSystem>Windows XP</operatingSystem>
@@ -447,7 +447,7 @@
 		<languageCode>fr</languageCode>
 		<language>French</language>
 		<deviceType>Desktop</deviceType>
-		<deviceTypeIcon>plugins/DevicesDetection/images/screens/normal.png</deviceTypeIcon>
+		<deviceTypeIcon>plugins/Morpheus/icons/dist/devices/desktop.png</deviceTypeIcon>
 		<deviceBrand>Unknown</deviceBrand>
 		<deviceModel />
 		<operatingSystem>Windows XP</operatingSystem>
@@ -563,7 +563,7 @@
 		<languageCode>fr</languageCode>
 		<language>French</language>
 		<deviceType>Desktop</deviceType>
-		<deviceTypeIcon>plugins/DevicesDetection/images/screens/normal.png</deviceTypeIcon>
+		<deviceTypeIcon>plugins/Morpheus/icons/dist/devices/desktop.png</deviceTypeIcon>
 		<deviceBrand>Unknown</deviceBrand>
 		<deviceModel />
 		<operatingSystem>Windows XP</operatingSystem>

@sgiehl
Copy link
Member

sgiehl commented Apr 19, 2017

The mentioned diff should already be applied. But guess there might be changes in brand icons.

@Findus23
Copy link
Member Author

You are right that was the previous change. Seems like there are no brand icons in the test files.

$ grep -r "brand"
tests/System/expected/test___API.getReportMetadata_day.xml:             <name>Device brand</name>
tests/System/expected/test___API.getReportMetadata_day.xml:             <dimension>Device brand</dimension>
tests/System/expected/test___API.getSegmentsMetadata.xml:               <name>Device brand</name>

@Findus23
Copy link
Member Author

Yesterday I have replaced the most important brands with better icons: matomo-org/matomo-icons@583c1ad

It was easier than I thought because Wikimedia has many of them (even as svg).

@sgiehl
Copy link
Member

sgiehl commented May 5, 2017

@Findus23 guess that would be good to merge. Do you have time to rebase and update the expected screenshots and remaining system test files?

@Findus23
Copy link
Member Author

Findus23 commented May 5, 2017

@sgiehl I'll finish it as far as I'm able to.

Notes:

I hope I didn't destroy anything in git lfs with bd803d5

@sgiehl
Copy link
Member

sgiehl commented May 5, 2017

Those changes are caused by the precise image update of travis. Guess we simply need to update those screenshots as well

@Findus23
Copy link
Member Author

Findus23 commented May 5, 2017

Two more things:

Failed asserting that 568941 matches expected 568880.

I've seen them before. Can I just update the number? I tried to download the system test artifacts but wget http://builds-artifacts.piwik.org/piwik/piwik/system.23045.tar.bz2 just hangs.

@sgiehl
Copy link
Member

sgiehl commented May 5, 2017

Two more things:

This pull request also updates some other icons as I have improved the conversion from svg files in the meantime: matomo-org/matomo-icons@3937b5d...1b55977

No problem. Simply update those tests as well.

There are four failing system tests left.

Failed asserting that 568941 matches expected 568880.
I've seen them before. Can I just update the number? I tried to download the system test artifacts but wget http://builds-artifacts.piwik.org/piwik/piwik/system.23045.tar.bz2 just hangs.

No, the files needs to be updates. That message occurs for non xml files like csv or pdf.
As comparing those files doesn't always work, we compare the size

@Findus23
Copy link
Member Author

Findus23 commented May 5, 2017

No, the files needs to be updates. That message occurs for non xml files like csv or pdf.
As comparing those files doesn't always work, we compare the size

That makes sense. But I can't replace them as the download from builds-artifacts.piwik.org doesn't work and I the still haven't looked into running the tests locally.

Could you commit them? (The branch is called move-old-brand-icons-to-piwik-icons)

@sgiehl
Copy link
Member

sgiehl commented May 5, 2017

I'm able to download the file in the browser. I'll update those files

@sgiehl
Copy link
Member

sgiehl commented May 5, 2017

Ok. Now all tests seem to run again 👍

@sgiehl sgiehl added this to the 3.0.4 milestone May 5, 2017
@Findus23 Findus23 added the c: Design / UI For issues that impact Matomo's user interface or the design overall. label May 6, 2017
@sgiehl
Copy link
Member

sgiehl commented May 6, 2017

had another rough look through all changes. Looks good. Will merge now 🎉

@sgiehl sgiehl merged commit a4f8566 into 3.x-dev May 6, 2017
@sgiehl sgiehl deleted the move-old-brand-icons-to-piwik-icons branch May 6, 2017 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants