Closed Bug 1047275 Opened 10 years ago Closed 10 years ago

Test ellipsis side on RTL pages for FontSizeManager

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: rik, Assigned: dwi2)

References

Details

Attachments

(1 file)

https://github.com/mozilla-b2g/gaia/blob/e554e48e95f2af0816be13a0976a3899c4545ec0/shared/js/dialer/font_size_manager.js#L99

We're missing a test to check that we're putting the ellipsis on the correct side for RTL languages.
Assignee: nobody → tzhuang
Attached file Pull request
Hi Rik,

Would you mind reviewing my patch? :)

Thanks
Attachment #8469882 - Flags: review?(anthony)
Comment on attachment 8469882 [details] [review]
Pull request

Redirecting to Doug.
Attachment #8469882 - Flags: review?(anthony) → review?(drs+bugzilla)
Comment on attachment 8469882 [details] [review]
Pull request

This patch could use some rethinking. Here's how I would structure it:

* MockL10n should be loaded with the rest of the MockXXX files, in the root `suiteSetup()`, and then unloaded again in the root `suiteTeardown()`. If there isn't a `mTeardown()` method, we should add one so that the LTR/RTL setting gets cleared for each test.

* Instead of adding two new tests for RTL, we can re-use the existing tests and pass LTR/RTL into them. Here's an example of what I mean:

```
['ltr', 'rtl'].forEach(function(direction) {
  test('adds ellipses to the end - ' + direction,
  function() {
    MockL10n.language.direction = direction;

    /* ... */

    FontSizeManager.adaptToSpace(
      FontSizeManager.DIAL_PAD, view, true,
      direction == 'ltr' ? 'end' : '');
  });

  test(/* ... */);
});
```

You can stick also stick these tests in a new combined "ellipses in LTR/RTL modes" suite.

The advantage of this is that it requires less code, and it's easier for reviewers and people new to the code to know that they should be supporting RTL, and how to do it. If we stick nearly-identical code in another suite, it can easily be missed.

Some additional comments:

* There needs to be more whitespace. Please keep the same code style as the code surrounding the changes. In particular, all `test()`, `suite()`, `setup()`, etc. calls should have a line break after them.

* The comment is a little bit inspecific and didn't immediately help me understand the code below it. I would suggest writing something like "MockL10n does not invert text when in RTL mode, but FSM will treat the right side as the beginning for the purpose of ellipses."
Attachment #8469882 - Flags: review?(drs+bugzilla) → review-
Comment on attachment 8469882 [details] [review]
Pull request

Hi Doug,

Thanks for reviewing. I addressed your comment and also did some refactoring on new unit tests I introduced in bug 1047998.

Thanks again.
Attachment #8469882 - Flags: review- → review?(drs+bugzilla)
Comment on attachment 8469882 [details] [review]
Pull request

This is better, especially structurally, but there are still some problems. See the PR for my comments.
Attachment #8469882 - Flags: review?(drs+bugzilla) → review-
Status: NEW → ASSIGNED
Target Milestone: --- → 2.1 S2 (15aug)
Comment on attachment 8469882 [details] [review]
Pull request

Hi Doug,

I've addressed your comment. However I am not quite sure if I get your idea on `viewElements`. So I leave my explanation on github.

Thanks again.
Attachment #8469882 - Flags: review- → review?(drs+bugzilla)
Comment on attachment 8469882 [details] [review]
Pull request

I left some more comments on the PR.
Attachment #8469882 - Flags: review?(drs+bugzilla) → review-
Comment on attachment 8469882 [details] [review]
Pull request

Hi Doug,

Comment addressed. Thanks
Attachment #8469882 - Flags: review- → review?(drs+bugzilla)
Comment on attachment 8469882 [details] [review]
Pull request

Looks good now. I left a few last comments for whitespace issues.

In the future, please fix review comments in a new commit so that it's easier to see what you've changed. You can squash them all once you're ready to land your patch.
Attachment #8469882 - Flags: review?(drs+bugzilla) → review+
Thanks, I'll keep that in mind.

landed on master
https://github.com/mozilla-b2g/gaia/commit/f07a308156862045aac8197698e33888c89452c7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: