Closed Bug 964036 Opened 11 years ago Closed 11 years ago

(gaia-rtl) Support RTL for Clock App

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Clock
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
2.1 S1 (1aug)

People

(Reporter: nefzaoui, Unassigned, NeedInfo)

References

Details

(Whiteboard: [rtl-meta])

Attachments

(3 files, 1 obsolete file)

Screenshot - Clock App RTL Issue
34.02 KB, image/png
Details
964036.patch
1.79 KB, patch
nefzaoui
: review-
Details | Diff | Splinter Review
https://github.com/mozilla-b2g/gaia/pull/21414
46 bytes, text/x-github-pull-request
nefzaoui
: review+
praghunath
: approval-gaia-v2.0-
Details | Review
This bug is set for tracking and fixing everything related to Right-To-Left issues in Clock App.
Blocks: gaia-rtl
Currently not so many bugs in Clock App Screenshot of the current status.
Attached patch 964036.patch — — Splinter Review
Hey, I believe this patch will fix the problem, added Vivien to review it.
Attachment #8444141 - Flags: review?(21)
Hey Mack, Just to give you a heads up of how this bug should be done In the list of Alarms LTR view has: 1) Alarm Time on the left 2) Alarm Name right before the middle, towards the left 3) Alarm Switch on the right RTL view should have: 1) Alarm Time on the right 2) Alarm Name right before the middle, towards the right 3) Alarm Switch on the left Also, please don't change original values, and instead add html[dir="rtl"] you_selector { only-property-that-needs-to-be-changed: its-new-value; } e.g. // LTR view >.x { > left: 0; >} We add att the end of the CSS file: >html[dir="rtl"] .x { > left: auto; > right: 0; >} md.otoum, You add your pull request as an attachment and look for "review" flag, choose ? and select the person who should review your PR. :)
Attached file Bug 964036- Support RTL for Clock App (obsolete) —
Attachment #8446276 - Flags: review?(21)
Comment on attachment 8444141 [details] [diff] [review] 964036.patch Review of attachment 8444141 [details] [diff] [review]: ----------------------------------------------------------------- Ahmed, do you have cycle to review those patches ? I can help in order to merge the patches if needed.
Attachment #8444141 - Flags: review?(21) → review?(nefzaoui.ahmed)
Comment on attachment 8444141 [details] [diff] [review] 964036.patch Review of attachment 8444141 [details] [diff] [review]: ----------------------------------------------------------------- Hey Mack, Thanks for your patch, unfortunately I can't give you an r+ for the following reason > #alarm-new { > position: absolute; > top: 0; >- right: 0; >- >+ left: 0; When implementing something that will be exclusively be shown in RTL view, you shouldn't edit the original bits of code. But instead you copy your css rule and add html[dir="rtl"] to it (right before the selector name), and make your changes there. This way when the html file has <html dir="rtl">, your RTL rule will be prioritized over the original one. Your changes are pretty much what we need for this bug, however they should be declared in separate RTL rule. Also please refer to Comment 4. Would love to see you make these changes :) Thanks!
Attachment #8444141 - Flags: review?(nefzaoui.ahmed) → review-
Comment on attachment 8446276 [details] [review] Bug 964036- Support RTL for Clock App Hey md.otoum Thanks for the patch. I left few comments on your Pull Request, your patch basically working, and that's what we need, however there are few nits that need to be taken care of. Also, can you handle mirroring the animations between clock tabs? If you can't I will open a follow-up bug to work on them separately and get a PR from here merged ASAP. I'll give a r- for now, please flag me for review? again once the issues are addressed. Thanks!
Attachment #8446276 - Flags: review?(nefzaoui.ahmed) → review-
Hey Ahmad, Thank you for your comments ill be getting it fixed asap I'll select you for review. Thank you for baring up with me :) I can handle the animation and I will do it Thank you again
im terribly sorry for this delay i had a lot of set backs but i hope everything is fine now :) much love and respect for ahmad who's always offered help :)
Attachment #8446276 - Attachment is obsolete: true
Attachment #8451350 - Flags: review?(nefzaoui.ahmed)
Hello Mohamad, Thank you for your patch, I left you couple of comments in your PR. Your patch satisfies what this bug requires, however there's still room for improvements in code quality + one little nit. It's basically an r+ but I'll hold on for now until we get those nits fixed. :) Please notify me when you get them done. Thanks for all your efforts.
hallo Ahmad , i just pushed the last version please have a look at it thanks :) cheers
Comment on attachment 8451350 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21414 Thank you for your contribution, this looks good now. There are couple of missing-space-after-colon nits but we can live with that for now. r+ but there's always a room for more improvements. Thank you Mohammad!
Attachment #8451350 - Flags: review?(nefzaoui.ahmed) → review+
Fixed the nits, squashed and rebased as requested by Ahmed. :)
This is ready ^
Flags: needinfo?(21)
Comment on attachment 8451350 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21414 NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] I am requesting approval-gaia-v2.0? for Ahmed and Mohammed, the community developers on this who sought guidance on flag use. Flagging gnarf as well to double-check our flagging manners and advise on uplift risk. If this cannot make it into 2.0 then 2.1 is fine. Thanks for the consideration! This fixes Tako and SPRD issues. [Bug caused by] (feature/regressing bug #): N/A [User impact] if declined: Arabic users won't have a readable clock. Tako and SPRD will be disappointed, and some additional partners with whom we are currently in conversation will be more difficult to secure. [Testing completed]: Ahmed and Mohammed can clarify; I'm not certain. [Risk to taking this patch] (and alternatives if risky): Low risk; mostly alignment changes to existing UI elements. Not big change. [String changes made]: None that I'm aware of. Believe strings stayed the same and only alignment and position of UI elements changed. Ahmed, please clarify if that is NOT the case.
Attachment #8451350 - Flags: approval-gaia-v2.0?
Flags: needinfo?(gnarf37)
Flags: needinfo?(bbajaj)
Flags: needinfo?(21)
Comment on attachment 8451350 [details] [review] https://github.com/mozilla-b2g/gaia/pull/21414 Late to accept any new string changes in release 2.0
Attachment #8451350 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0-
Flags: needinfo?(bbajaj)
There is no string changes in this patch.
Keywords: checkin-needed
seems travis failed here - https://travis-ci.org/mozilla-b2g/gaia/builds/29745477 could you take a look if this errors are related to your patch ? thanks!
Keywords: checkin-needed
From a total 4, 3 are related to Email and 1 from Vertical Homescreen. Doesn't seem like related to this patch since this one doesn't change any of the original code and just adds css that will be loaded only when an RTL language is selected.
Flags: needinfo?(cbook)
ok if the failures are unreleated than its ok master: https://github.com/mozilla-b2g/gaia/commit/c8511217183ade630f5afa7698c04efef5c6806b
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(cbook)
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S1 (1aug)
[Environment] Gaia 3584b2723412ed3299c6761f465885d80651c87e Gecko https://hg.mozilla.org/mozilla-central/rev/e7806c9c83f3 BuildID 20140820160201 Version 34.0a1 ro.build.version.incremental=94 ro.build.date=Tue May 20 09:29:20 CST 2014 [Result] PASS
Status: RESOLVED → VERIFIED
Mass Edit: adding the [rtl-meta]
Whiteboard: [rtl-meta]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: