Skip to content
This repository has been archived by the owner on Nov 27, 2019. It is now read-only.

l10n title/description for install.rdf like nodejs #495

Merged
merged 9 commits into from
Feb 16, 2016
Merged

l10n title/description for install.rdf like nodejs #495

merged 9 commits into from
Feb 16, 2016

Conversation

jc3213
Copy link
Contributor

@jc3213 jc3213 commented Feb 16, 2016

This is based on #419 , But the original one have more than one additional functions and most of them need more tweak to be done. So I've rewritten the l10n part according to jpm code style(maybe?)

This is based on #419
The original one have more than 1 additional functions and needs more tweak, so I've rewritten the l10n part according to jpm code style(maybe?)
@jc3213
Copy link
Contributor Author

jc3213 commented Feb 16, 2016

Here's a sample to test the functional https://github.com/jc3213/jpm/releases/download/1.0.5/test.zip
Sorry that I don't have a github client, and can't attach files to the comment(due to my network provider)

@kumar303 Will you please have a look into this?

There's also contributors, translators, and developers tags too, but it needs more tweak to make this part work well.
This also reverted last commit https://github.com/jc3213/jpm/commit/925d2979c7e6f3d29fe172bf814ad6c5be57f7d6
@johannhof
Copy link

Yay, thanks for doing this. 👍 This would fix #479. You should probably add a test if this works properly, though.

@jc3213
Copy link
Contributor Author

jc3213 commented Feb 16, 2016

@johannhof That's been done, please have a look into it

@jc3213
Copy link
Contributor Author

jc3213 commented Feb 16, 2016

I've added the original title and description in order to fulfill the complete test that if only title/description get localized. If so get original description/title.

For example: "locales": "ja": {"title": "ja localed title"} or "locales": "ja": {"description": "ja localed description"}

var l10nDescription = {
"em:locale": locale,
"em:name": l10n.title || manifest.title || manifest.name || "Untitled",
"em:description": l10n.description || manifest.description || undefined,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: || undefined can be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did keep that to match line 52 "em:description": manifest.description || undefined, I think It should be better to let it this way

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, no idea what jsontoxml does for other falsy values, so it's probably best to leave it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed them to jetpackMeta["em:name"] and jetpackMeta["em:description"], and in this way its faster than calculate the name and description again

it failed on `npm run jscs` but works on `npm test unit` and could build add-on correctly
@jc3213
Copy link
Contributor Author

jc3213 commented Feb 16, 2016

@johannhof @kumar303 this pull requests couldn't solve contributors, developers, and translators tags, I've done another one but the generated install.rdf has too much CRLF but it supports contributors, developers, and translators tags. See https://github.com/jc3213/jpm/commit/f3e65852c468d4a01544db1f0e0643c38b749ec2

According to document on MDN about package.json https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/package_json It seems there is no need to support contributors, no info about developers and do support translators. It's really a headache

          <em:localized>
            <Description>
                <em:locale>ja</em:locale>

                <em:name>ja addon name</em:name>

                <em:description>ja addon desc</em:description>

                <em:creator>author</em:creator>

                <em:homepageURL>homepage</em:homepageURL>

</Description>
</em:localized>

}
});
var locales = xml.getElementsByTagName("em:localized");
var locale_ja = locales[0].childNodes[1]; // Description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all variables typically follow camel case format, such as localeJa.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kumar303 That has been done

@kumar303
Copy link
Contributor

Thanks for all your work on this and sorry about the headaches

kumar303 added a commit that referenced this pull request Feb 16, 2016
l10n title/description for install.rdf like nodejs
@kumar303 kumar303 merged commit 92a3ab9 into mozilla-jetpack:master Feb 16, 2016
@asamuzaK
Copy link
Contributor

Thanks for the fix.
It should be documented too.
package.json - Mozilla | MDN

@kumar303
Copy link
Contributor

Yes. Could someone edit MDN to add documentation? It's like a wiki so anyone can edit it.

@SebastianZ
Copy link

If I read the patch correctly, it allows to add the localization directly within the package.json using the locales property.
Though it doesn't allow to have the translations within the .properties files as initially requested, i.e. as <add-on name>.title=Title and <add-on name>.description=Description. Is that correct?
And if so, should I create a new issue for changing that/adding this as a second way?

Sebastian

@kumar303
Copy link
Contributor

yes, please create a new issue

@SebastianZ
Copy link

Thanks for the quick answer! I've created #527.

Sebastian

@SebastianZ
Copy link

Thanks for the fix.
It should be documented too.
package.json - Mozilla | MDN

Done:

Sebastian

@kumar303
Copy link
Contributor

The docs look great! Thank you

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants