Code review comment for lp:~madsrh/ubiquity-slideshow-ubuntu/Yaru-CSS

Revision history for this message
MadsRH (madsrh) wrote :

Thank you for the reply Sebastien!

> the commit message/description doesn't really convey why we need those changes.
> why are the css changes needed?

The current ubiquity slideshow uses colors from Ambiance for background, hyperlink and title. These were updated to match the new theme "Yaru".

> In fact the messages speaks about color but the diff includes new icons?

Sorry about that. The icons needs updating too because the new theme ships with Suru icons.
This is my very first commit EVER using Launchpad! I'm not a developer, but the though of seeing the new theme (that we spend 11 months working on) mixed with Ambiance styling, was so depressing that I read up on Launchpad documentation to push this fix myself.
I understand now, that I should have added the icons in a seperate MR or at least added them to the description.

> where are the icons coming from?

Sams Suru repo https://github.com/snwh/suru-icon-theme/tree/master/Suru

> - we already got a recent icons refresh with ... how is that different?

I simply didn't know! I tried the daily build and it still uses the old icons. I'll remove the Chromium and Firefox and only leave the Suru icons. Should I remove all the icons? Do you want a seperate MR for these?

> also the file permissions changes to +x on the .png seems buggy

Again, I didn't know, but it is noted that I need to check file permissions. I was just keen to push this because no one else did.

> Sorry that you felt like your work is not being considered, it's late for changes but we all want the best result so let's see what can be done.

+1 We really do and don't be sorry. Your workflow (you, as in Canonical) uses Launchpad, we use Github and the Community Hub (the mockups and code was there for months), so it's just a matter of learning to working together.

« Back to merge proposal