Code review comment for lp:~uriboni/webbrowser-app/find-in-page

Revision history for this message
Olivier Tilloy (osomon) wrote :

Thanks for working on this. I did a first quick round of testing review, here are a few comments, in no particular order:

The flake8 unit test is failing.

The label of findInPageCounter should be i18n’d, with a proper TRANSLATORS comment.
As a consequence, autopilot tests shouldn’t rely on the value of that label. Can we somehow expose the values of current and count as properties of the label, for testing purposes?

It appears findInPageMode is never assigned a default value. Not sure whether javascript (and its QML implementation) specifies that booleans are false by default, but it would be cautious to have it default to false.

87 + value: textField.text.length > 1 ? textField.text : ""
  Shouldn’t the above be [textField.text.length > 0] ? Or is it intentional that searching doesn’t start for one single character? I can’t open the UX spec right now, if this is intentional would you mind adding a comment to explain the rationale?

« This is because the UI Toolkit component TextField does not support adding components after the clear button »
  Can you raise that with design (James Mulholland and the UITK team, to ensure they are aware of the issue?

The size of ChromeButton instances in Chrome.qml has been updated at revision 997, can you please update the new instances too?

In the autopilot emulator for the chrome, the new buttons could use a more explicit naming scheme, e.g. "find_next_button()" and "find_prev_button()".

review: Needs Fixing

« Back to merge proposal