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

Package "mass icons" via composer or submodule #11370

Closed
Findus23 opened this issue Feb 20, 2017 · 27 comments
Closed

Package "mass icons" via composer or submodule #11370

Findus23 opened this issue Feb 20, 2017 · 27 comments
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Milestone

Comments

@Findus23
Copy link
Member

Currently I maintain all "mass icons" (browsers, os, flags, ...) in my repository https://github.com/Findus23/device-icons and create pull requests against the main piwik repository on changes.
I want to change this to directly using the repository as

  • it makes maintaining easier as someone who wants to add a icon only need to create a pull request agianst the repository
  • it doesn't bloat the main repository as binary files don't diff nicely

While submodules would be easier for testing, it seems like they are only used for optional plugins in piwik. And it would add the whole repository (not just the resized images), but this could be solved in the build script.
On the other hand, it's possible to exclude all unneeded files from github zips (see https://github.com/Findus23/device-icons/archive/1.1.0.zip), which in theory should also exclude them from composer packages. (I haven't tested it)

What would you prefer?
(PS: In both cases it might be useful to move it to the piwik organisation)

@mattab
Copy link
Member

mattab commented Feb 20, 2017

Hi @Findus23

  • Using a submodule would be OK for me. (we don't only use them for plugins for example, we use submodule for Log Analytics @ https://github.com/piwik/piwik-log-analytics in piwik/misc/log-analytics)
  • Would you maybe consider migrating your device-icons repository to our Piwik organisation?
  • we'll have to exclude the files from the ZIP archives (1) using gitattributes as well as 2) updating our package build script adding a rm command to the long list)

Maybe other people have other thoughts? cc @tsteur @sgiehl

@Findus23
Copy link
Member Author

Would you maybe consider migrating your device-icons repository to our Piwik organisation?

As mentioned I don't mind but currently I am not able to as I don’t have the permission to create repositories on piwik.

we'll have to exclude the files from the ZIP archives (1) using gitattributes as well as 2) updating our package build script adding a rm command to the long list)

Are you sure that (1) is necessary as it seems like Github doesn't include submodules in ZIP archives. (just empty directories)

@sgiehl
Copy link
Member

sgiehl commented Feb 20, 2017

As mentioned I don't mind but currently I am not able to as I don’t have the permission to create repositories on piwik.

You should be able to do that nevertheless. Within your repo setting you should have possibility to "transfer ownership". That way you should be able to move it to piwik.

@Findus23
Copy link
Member Author

Oddly it seems like this isn't possible

Users must have admin or owner permissions within the receiving organization before they can transfer a repository that they individually own.
https://help.github.com/articles/transferring-a-repository-owned-by-your-personal-account/#transferring-to-an-organization

But I should be able to transfer it to you and then you can transfer it to piwik.

@sgiehl
Copy link
Member

sgiehl commented Feb 20, 2017

Got a request for transfer, but can't accept it. Getting a 500 error when visiting the confirmation page. But you might add me as collaborator within your repo settings. Maybe I'm able to transfer it on my own.

@Findus23
Copy link
Member Author

Getting a 500 error when visiting the confirmation page
That's really strange.

BTW: While we're on it, has anyone an idea for a better name? device-icons doesn't really fit anymore.

@sgiehl
Copy link
Member

sgiehl commented Feb 20, 2017

Nope. doesn't work either. Seems I can't enter the settings as collaborator.
I could create a new repo under piwik with the same name and give you push access, so you can push the repo there.

@Findus23
Copy link
Member Author

I could create a new repo under piwik with the same name and give you push access, so you can push the repo there.

That would work

@sgiehl
Copy link
Member

sgiehl commented Feb 20, 2017

Just created it. You should have admin access to this new repo https://github.com/piwik/device-icons

@Findus23
Copy link
Member Author

Findus23 commented Feb 20, 2017

EDIT: Forgot to accept invite

@Findus23
Copy link
Member Author

Seems to be working

@mattab
Copy link
Member

mattab commented Feb 20, 2017

Name: how about piwik-icons? 😎

@Findus23
Copy link
Member Author

Findus23 commented Feb 20, 2017

how about piwik-icons? 😎

Sounds good (and descriptive)
Which creates a new question: Where would you add the submodule? /plugins/icons/? It isn't really a plugi.

@mattab
Copy link
Member

mattab commented Feb 20, 2017

How about misc/icons ?

@sgiehl
Copy link
Member

sgiehl commented Feb 20, 2017

we could also add that within an plugin like plugins/Morpheus/icons.
Not sure if some people might restrict access to the misc folder, as it's not really needed. Only for a custom logo afaik.

@mattab
Copy link
Member

mattab commented Feb 20, 2017

I didn't think of the fact that icons will be loaded from the Web app UI. I thought that we would still copy all icons to the right places / in the right plugins folders ?

@sgiehl
Copy link
Member

sgiehl commented Feb 20, 2017

Sue, if we copy them, it's not a problem. Not sure how it will be handled...

@Findus23
Copy link
Member Author

I'm not sure if we should copy them as this creates many directories whose source isn't obvious.
I'd prefer plugins/Morpheus/icons

@mattab
Copy link
Member

mattab commented Feb 20, 2017

By copying I meant only copy those directories within dist https://github.com/piwik/device-icons/tree/master/dist into these directories where the icons currently reside (as per the dist sub-listings). ie. could be done by running a simpleshell script. The idea would be to keep a meaningful icons path where icons would still sit in the related plugins and not all in Morpheus theme.

But maybe that's not too useful and we could put all icons in the Morpheus theme.

@Findus23
Copy link
Member Author

I agree plugins/Morpheus/icons/dist/DevicesDetection/images/os/AND.png is overly complicated and redundant. Maybe copying is really simpler. Only disadvantage is that everybody, who wants to use (or develop) piwik from git, needs to run this script. So it could help if it isn't a bash script, but a cli php script.

@mattab
Copy link
Member

mattab commented Feb 20, 2017

Only disadvantage is that everybody, who wants to use (or develop) piwik from git, needs to run this script. So it could help if it isn't a bash script, but a cli php script.

Good point re: PHP, like a console command in the development: scope where we already have similar commands eg. development:sync-system-test-processed / development:sync-ui-test-screenshots.

maybe not everybody would have to run this script, as it would only be run when some icons are updated or so, which would be not so frequent and likely done by just a few team members.

@Findus23
Copy link
Member Author

maybe not everybody would have to run this script, as it would only be run when some icons are updated or so, which would be not so frequent and likely done by just a few team members.

Yes, but everyone (also travis) needs to run it once after git clone, otherwise they don't see any icons.

@mattab
Copy link
Member

mattab commented Feb 20, 2017

we need those icons in the git repo without having to run any command, because it's expected that a fresh Piwik git clone will just work out of the box. So we'll have to add/push them to the repo as well.

So far I see two options

  • if we don't use the submodule, we would just a console command to copy those files in Morpheus
  • if we use a submodule we can put all icons in Morpheus

@Findus23
Copy link
Member Author

If we add them to the git repository it gets much more complicated.
But we can go the opposite way:

That way a git clone (with clone of sub directories "just work") and we only need to remove the source images on build.

@mattab
Copy link
Member

mattab commented Feb 21, 2017

@Findus23 Your last proposal sounds good and best so far 👍 I think it would work nicely!

@mattab mattab added this to the 3.0.3 milestone Feb 21, 2017
@mattab mattab added the c: Design / UI For issues that impact Matomo's user interface or the design overall. label Feb 21, 2017
@mattab
Copy link
Member

mattab commented Mar 28, 2017

New piwik-icons repository and project: https://github.com/piwik/piwik-icons 🎉

@mattab
Copy link
Member

mattab commented Mar 28, 2017

Done in All icons in own repo and included as a submodule #11548 and later in #11272

Closing but in case i've missed something please reopen.

@mattab mattab closed this as completed Mar 28, 2017
@mattab mattab added the RFC Indicates the issue is a request for comments where the author is looking for feedback. label Apr 3, 2017
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. RFC Indicates the issue is a request for comments where the author is looking for feedback.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants