Code review comment for lp://staging/~stefan-schwarzburg/qreator/qreator_sms_mms_tel

Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote :

> In terms of functionality, it looks good to me. I've got a few recommendations
> in terms of layout and wording. Rather than just listing them, I thought I'd
> put them on a branch for you to review and if you're happy with them to accept
> them:
>
> bzr push lp:~dpm/qreator/qreator_sms_mms_tel
>
> ## Rationale
>
> - Layout: I've just added some horizontal and vertical spacing to the widgets
> to be consistent with the rest of the code's UI

Thanks. Maybe you could add a short HACKING file or README file, where we can add the typical layout properties?
Something like:
- Entry: margin xxx, align xxx, ...,

I have of cause accepted your changes!

> - Wording: I've modified some texts with the intention to make them
> (hopefully!) more user-friendly. I've avoided using SMS other than in the
> combobox and I've suggested "Messaging", as as far as I know, at least Brits
> tend not to use the acronym (they use "text messaging"), and looking at my
> Android phone there is no reference to SMS at all (only to "Messaging")

You are right to go for consistency. I just hope the German translation will be SMS and not "Nachricht" or "Text Nachricht"...

> - Other: I've removed the extra tooltip with the warning regarding some code
> readers not suporting the SMS format, as we're already clearly stating that in
> the placeholder text
>

Sure.

> Finally, this is not a requirement to pass this review, but on future merge
> proposals, and again for the sake of consistency with the rest of the code, if
> you could look into wrapping the code at 80 lines, that'd be great, thanks!
>

Actually, this should be a requirement :-)
I have now changed this, as well as all other pep8 violations and pylint warnings/errors.

> I'm setting this as Needs Fixing for now, as I think at the very least the
> layout changes should be applied.
>
> Let me know what you think.
I think qreator is really getting forward :-)

« Back to merge proposal