Remove "R$" from all non-portuguese-BR keyboard layout

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: timdream, Assigned: bruno)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=rudyl][mentor=timdream][mentor-lang=zh][lang=js])

Attachments

(1 attachment)

47 bytes, text/x-github-pull-request
timdream
: review+
Details | Review | Splinter Review
(Assignee)

Comment 1

5 years ago
I will try to fix this as my first bug. :)

I just need to remove "[r: 'R$ ',]" from file, right?
Flags: needinfo?(nobody)
We do have 'R$' as one of the alternative char for 'r', when referencing the original spec for this.
https://docs.google.com/spreadsheet/ccc?key=0Aho3t5kX1TtRdFFOS25rZFNXcncxZm9LRUtzaDlKLVE#gid=0

For '$', I am not sure about it, this was added back in Bug 807846.

Tim, do you have any strong opinion to remove these alternative characters or we will need ask for UX input?
Thanks.
Flags: needinfo?(timdream)
Flags: needinfo?(nobody)
(In reply to Rudy Lu [:rudyl] from comment #2)
> For '$', I am not sure about it, this was added back in Bug 807846.

If Brazilian Portuguese keyboard need that in the alternate layout, we should give a specific alternate layout.
Flags: needinfo?(timdream)
(Assignee)

Comment 4

5 years ago
R$ as alternative for R is also present on files:
ca.js
es.js
hr.js
n1.js

There are, also, alternatives for '$':
'$': '€ £ ¥ R$',
The desire result of this bug is

-- For pt-BR, R$ present as alt chars for $ and R keys.
-- For all other languages, including English, removing R$.

Bruno, are you still taking this bug? I didn't set my Bugzilla query right so I missed your last comment, sorry about that.
Assignee: nobody → bruno
Flags: needinfo?(bruno)
(Assignee)

Comment 6

5 years ago
 Sorry for the delay. I was traveling.

> Bruno, are you still taking this bug? I didn't set my Bugzilla query right
> so I missed your last comment, sorry about that.

 I was waiting for the confirmation that I could work on it. I will do the changes and send the patch, ok? Could you, please, assign it to me?
Flags: needinfo?(bruno)
(In reply to Bruno Vilar from comment #6)
>  Sorry for the delay. I was traveling.

No worries :)

> > Bruno, are you still taking this bug? I didn't set my Bugzilla query right
> > so I missed your last comment, sorry about that.
> 
>  I was waiting for the confirmation that I could work on it. I will do the
> changes and send the patch, ok? Could you, please, assign it to me?

You are already the assignee :)
(Assignee)

Comment 8

5 years ago
Created attachment 8361157 [details] [review]
patchForBug943812.html
Attachment #8361157 - Flags: review?(timdream)
Comment on attachment 8361157 [details] [review]
patchForBug943812.html

The patch looks correct. Thank you very much.
Attachment #8361157 - Flags: review?(timdream) → review+
master: https://github.com/mozilla-b2g/gaia/commit/4c4cbcc3683177089135877dcc4602a01f2d1997
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Remove "R$" from English keyboard layout → Remove "R$" from all non-portuguese-BR keyboard layout
Actually, the patch did not remove R$ as alternatives for '$' in many non-pt-BR layouts:

timdream:~/repo/gaia master$ grep -r 'R\$' keyboard/layouts/
keyboard/layouts/bn-Avro.js:      '৳': '$ € £ ¥ R$',
keyboard/layouts/bn-Probhat.js:      '৳': '$ € £ ¥ R$',
keyboard/layouts/ca.js:      '€': '$ £ ¥ R$',
keyboard/layouts/en.js:      '$': '€ £ ¥ R$',
keyboard/layouts/es.js:      '€': '$ £ ¥ R$',
keyboard/layouts/my.js:      '€': '€ ¥ R$'
keyboard/layouts/pt-BR.js:    r: 'R$ ',
keyboard/layouts/pt-BR.js:      'R$': '€$£¥',
keyboard/layouts/pt-BR.js:        { value: 'R$', compositeKey: 'R$' }, { value: '&' },
timdream:~/repo/gaia master$

I am sorry for not seeing them during review. Could you file another follow-up bug and fix those? Thank you very much!
Flags: needinfo?(bruno)
(Assignee)

Comment 12

5 years ago
Tim, I'm sorry. Had seen in 'R$' in those places, but I kept it because I thought it was expected, considering the other currency symbols.

I'll remove and send a new patch soon.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

5 years ago
Ops.. I have to file another bug. I will try to do it.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 960955
(Assignee)

Comment 14

5 years ago
New bug created: https://bugzilla.mozilla.org/show_bug.cgi?id=960955.

Patch submited. 

If I did something wrong, please, tell me. I really want to understand how to create and solve bugs in a proper way. :)
Flags: needinfo?(bruno)
Duplicate of this bug: 978289
You need to log in before you can comment on or make changes to this bug.