Code review comment for lp://staging/~danielmcguire/help-app/help-app

Revision history for this message
Daniel McGuire (danielmcguire) wrote :

Hello David,

After a painful evening last night with dobey and balloons from the IRC
I did manage to get something into LP, when I did it seemed that it
missed the entire WWW folder, this is where I made all the changes.

My lack of LP and BZR is quite a roadblock at the moment.

We had a massive problem last night that one of the .PO files changed on
its own and I had to manually go in there and revert it. I am not sure
how this happened.

If it helps I only really made 4 changes (excluding the added svg file):

CSS File: app/www/theme/css/app.css (updated with CSS)
index.en-us.html: app/www/ (updated)
get-in-touch.en-us.html: app/www/ (updated)
faq.en-us.html: app/www/ (updated)

Because the bzr push to LP didn't take the www folder none of the
changes I made happened. I think there is something I am missing.

I am happy to fix everything with a little guidance.

Daniel McGuire

On 12/03/15 05:41, David Planella wrote:
> Review: Needs Fixing
>
> Wow, thanks a lot Daniel!
>
> Looking at the diff, it seems the only relevant change matching the description is the addition of an SVG file. I think you might have forgotten to either commit or push the other CSS fixes to the branch?
>
> I've also added a few inline comments to the diff below, happy to have a chat on IRC if you need help.
>
> Diff comments:
>
>> === modified file 'Makefile' (properties changed: -x to +x)
> Making the Makefile executable (changing mode to +x) was probably unintended and not relevant to this merge proposal. Could you please revert this change?
>
>> === added file 'app/pictogram-quote-orange-hex.svg'
>> --- app/pictogram-quote-orange-hex.svg 1970-01-01 00:00:00 +0000
>> +++ app/pictogram-quote-orange-hex.svg 2015-03-11 20:56:23 +0000
>> @@ -0,0 +1,12 @@
>> +<?xml version="1.0" encoding="utf-8"?>
>> +<!-- Generator: Adobe Illustrator 15.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) -->
>> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
>> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
>> + width="141.732px" height="141.732px" viewBox="70.866 70.866 141.732 141.732"
>> + enable-background="new 70.866 70.866 141.732 141.732" xml:space="preserve">
>> +<g>
>> + <path fill="#DD4814" d="M170.479,190.605c-8.43,4.971-18.256,7.819-28.747,7.819c-31.312,0-56.693-25.382-56.693-56.694
>> + c0-31.311,25.382-56.693,56.693-56.693c31.31,0,56.692,25.382,56.692,56.693c0,10.328-2.762,20.008-7.585,28.346l-0.012-0.005
>> + l7.597,28.353l-28.354-7.597L170.479,190.605z"/>
>> +</g>
>> +</svg>
>>
>> === modified file 'edit-here/Makefile' (properties changed: -x to +x)
> Here's another change to the executable mode of the Makefile. Could you revert this one too?
>
>> === modified file 'edit-here/po/de.po'
>> --- edit-here/po/de.po 2015-03-11 05:50:31 +0000
>> +++ edit-here/po/de.po 2015-03-11 20:56:23 +0000
>> @@ -14,6 +14,7 @@
>> "MIME-Version: 1.0\n"
>> "Content-Type: text/plain; charset=UTF-8\n"
>> "Content-Transfer-Encoding: 8bit\n"
>> +
> Here's a one-line change to a .po file that is not relevant to this merge proposal and adds unnecessary noise to the diff. Would you mind reverting this one too? Thanks!
>
>> "X-Launchpad-Export-Date: 2015-03-11 05:50+0000\n"
>> "X-Generator: Launchpad (build 17389)\n"
>> "Language: \n"
>>
>

« Back to merge proposal