Evan Tseng [:evanxd] Assignee
	Description • 11 years ago

The refactor is extracting the tags screen from contacts.js into a module.

	James Lal [:lightsofapollo]
	Comment 1 • 11 years ago

Wrong component looks like.

	Evan Tseng [:evanxd] Assignee
	Comment 2 • 11 years ago

Hi James,

You're right.
Thanks. :)

	Evan Tseng [:evanxd] Assignee
	Comment 3 • 11 years ago

Hi Francisco,

Please help me review this patch.
Thanks. :)

	Francisco Jordano [:arcturus] [:francisco]
	Comment 4 • 11 years ago

Hi Evan,

first quick feedback, unit tests are broken for the fill tag options :S
Will continue taking a deep look to the PR.

Thanks!

	Francisco Jordano [:arcturus] [:francisco]
	Comment 5 • 11 years ago

A part from the previous feedback, I left two comments on github regarding two things we need to solve.

Once that's fixed, the functionallity is working quite well, so won't be any problem to merge :)

Great job Evan!

	Evan Tseng [:evanxd] Assignee
	Comment 6 • 11 years ago

Hi Francisco,

I added a unit test for the content_tag.js
It worked well.

And I updated my code in GitHub.
Please help me review the patch again.
Thanks. :)

	Francisco Jordano [:arcturus] [:francisco]
	Comment 7 • 11 years ago

Hi Evan,

sorry for being a pain in the ass :S, tried the latest version and when I change language, in the contact details I can see the tag updated, but when entereing in the tags screen everything appears in english.

Also leave another couple of comments in github, but in this case are minor things.

Could you try to reproduce that problem changing the languange and then going to the tag screen?

Thanks!

	Evan Tseng [:evanxd] Assignee
	Comment 8 • 11 years ago

Hi Francisco,

Thanks for finding out the changing language issue.
I already fixed it.

And the updated patch was already on GitHub.
Please help me review it.

Thanks. :)

	Francisco Jordano [:arcturus] [:francisco]
	Comment 9 • 11 years ago

Almost there!

Left you two more comments on github, once resolved I think we are done here.

Thanks a lot for your work Evan!

	Evan Tseng [:evanxd] Assignee
	Comment 10 • 11 years ago

Hi Francisco,

I updated my code, and I also left comments on GitHub.
Please review that.

Thanks. :)

	Evan Tseng [:evanxd] Assignee
	Comment 11 • 11 years ago

Hi Francisco,

Could the patch be merged in v1-train and v1.0.1 branch?

Thanks. :)

	Francisco Jordano [:arcturus] [:francisco]
	Comment 12 • 11 years ago

Here are the STR:

- Go to a contact.
- Edit the contact and change the phone label to any option that has two words, like 'Fax office'.
- Click on done.
- Save the contact.
- Go back to the contact details.
- Click on edit contact.

Expected:
The label in the edit form should show 'FAX OFFICE'

Actual:
We show 'FAXOFFICE'

This could be caused by the fact that the actual l10n identifier is 'faxoffice'

Thanks.

	Francisco Jordano [:arcturus] [:francisco]
	Comment 13 • 11 years ago

(In reply to Evan Tseng [:evanxd] from comment #11)
> Hi Francisco,
> 
> Could the patch be merged in v1-train and v1.0.1 branch?
> 
> Thanks. :)

Hi Evan,

I guess we are out of both release windows, for 1.0.1 and 1.1, anyway let's try to land this on master, and if the rest of the refactor is not much painful, and the improvement is to hight let's nominate it for v1.1.

Thanks!
F.

	Evan Tseng [:evanxd] Assignee
	Comment 14 • 11 years ago

Hi Francisco,

I got it.

And I'm fixing the issue.

Thanks. :)

	Evan Tseng [:evanxd] Assignee
	Comment 15 • 11 years ago

Hi Francisco,

I found out that the issue in Comment 12 was not related with this patch.
It was already happened in current Gaia master branch.

So we should create a new bug for the issue, and fix it.
And we could continue to review and land this refactoring patch.

How do you think? :)

	Francisco Jordano [:arcturus] [:francisco]
	Comment 16 • 11 years ago

Hi Evan,

definitely you are right, if that's happening in master is a different issue.

So I'll r+ this patch as everything was looking pretty good and let's create that new bug.

Thanks a lot for your hard work!