Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Localize add-on & add features for menu display #17

Merged
merged 35 commits into from
Mar 15, 2021
Merged

Conversation

rugk
Copy link
Owner

@rugk rugk commented Feb 28, 2021

Supersedes #5

I reverted some of the last commits (i.e. did not use them in this branch) and cleaned up the code afterwards.

See the commits for for information.

BTW here is a code snipping one can paste into the debug console to get the transformed entries of the translated versions of the transformation names:

fonts = await import("/common/modules/data/Fonts.js");
UnicodeTransformationHandler = await import("/common/modules/UnicodeTransformationHandler.js");
fonts.menuStructure.filter(x => x !== fonts.SEPARATOR_ID).map(id => {
	const translatedMenuText = browser.i18n.getMessage(id);
  return UnicodeTransformationHandler.transformText(translatedMenuText, id);
})

We may document that in the wiki or so. It helped me to update the German version, because it names some examples in the docu, this way, they are easy to get.


I'm sorry to make such a big PR, but I exaggerated a little as I just implemented two new features too:

  • you can show the description of the transformation without the applied transformation (as it may be hardly readable)
  • you can show the selected text itself as a "live preview" instead of a static string for each entry 🙂
  • you can combine these two

* add English translation as template
* add German translation
Tab to spaces in JS as per editorconfig.
from UnicodeFontHandler, which now solely handles the font/transformation stuff
Also fixes the issue that the "old" style was not reset when you
clicked in the context menu without any selected text.
@todo
Copy link

todo bot commented Feb 28, 2021

check Chrome/ium comaptibility here (this workaround should make it work for now)

// TODO: check Chrome/ium comaptibility here (this workaround should make it work for now)
if (menus.onShown) {
menus.onShown.addListener(handleMenuShown);
}
menus.onClicked.addListener(handleMenuChoosen);


This comment was generated by todo based on a TODO comment in 6466b50 in #17. cc @rugk.

@rugk rugk requested a review from tdulcet February 28, 2021 19:48
@rugk
Copy link
Owner Author

rugk commented Feb 28, 2021

Oh thanks TODO bot, yeah, @tdulcet you’ve tried that with Chromium. I guess for now it’s okay, if it works without errors. (Likely the second feature won’t be possible to be made to work, because Chrome/ium has no browser.menu.onShown callback (API).

@rugk rugk changed the title Localize add-on Localize add-on & add features for menu display Feb 28, 2021
@rugk rugk mentioned this pull request Feb 28, 2021
1 task
@rugk rugk added the i18n/l10n internationalisation & localisation label Feb 28, 2021
@rugk rugk added this to the 0.5-beta milestone Feb 28, 2021
Copy link
Owner Author

@rugk rugk left a comment

Choose a reason for hiding this comment

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

FYI

scripts/manifests/chromemanifest.json Show resolved Hide resolved
Copy link
Collaborator

@tdulcet tdulcet left a comment

Choose a reason for hiding this comment

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

I'm sorry to make such a big PR, but I exaggerated a little as I just implemented two new features too:

  • you can show the description of the transformation without the applied transformation (as it may be hardly readable)
  • you can show the selected text itself as a "live preview" instead of a static string for each entry 🙂
  • you can combine these two

Wow, thank you for adding these features! The "live preview" feature is cool.

Oh thanks TODO bot, yeah, @tdulcet you’ve tried that with Chromium. I guess for now it’s okay, if it works without errors. (Likely the second feature won’t be possible to be made to work, because Chrome/ium has no browser.menu.onShown callback (API).

I just tested this PR in Chrome and it does work without errors. You do need to temporally remove the icons from the manifest.json file to load it, as unlike Firefox, Chrome does not let users load extensions if any of the files do not exist. You also have to manually set the settings in the common/modules/data/DefaultSettings.js file until rugk/awesome-emoji-picker#4 (comment) is fixed (or make the change noted in that comment).

Overall, this PR looks very good! I only found a few minor issues. There are also some ESLint errors, which I did not mention in my review, including several additional ones after fixing this.

<input class="setting save-on-change" type="checkbox" id="livePreview" data-optiongroup="unicodeFont" name="livePreview">
<label data-i18n="__MSG_optionMenuShowLivePreview__" for="livePreview">Show a live preview with the selected text</label>
</div>
<span data-i18n="__MSG_optionMenuShowLivePreviewDescr__" class="line indent helper-text">Shows the effect of the transformation on the selected text instead of an example one.</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a note that this is only for Firefox/Thunderbird.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, I'd rather think of conditionally disabling it like the isMobile/ MobileHelper (or how it was called) does.
However, I did not want to put work into it yet, but it's definitively a thing that should be done before a Chrome/ium release.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, for now I've adjusted the AutomaticSettings to just handle it the same as mobile-incompatible settings and hide them. May be changed later though.

Copy link
Collaborator

@tdulcet tdulcet Mar 3, 2021

Choose a reason for hiding this comment

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

Thanks for updating your library. Note that the change will hide the option for Thunderbird, in addition to Chrome. This line:
https://github.com/TinyWebEx/AutomaticSettings/blob/f67aef00fed936456f009f29a7518d6b38056c78/EnvironmentalOptions.js#L36
would need to be:

return browserInfo.name === "Firefox" || browserInfo.name === "Thunderbird";

although maybe also rename the function to something like isMozilla or isGecko.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Should be fixed now, though I used the vendor name…

Copy link
Collaborator

Choose a reason for hiding this comment

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

The vendor name does not work on Thunderbird, since it is blank:
image

We should either submit a bug to them or change your library to use the name instead.

src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/background/modules/ContextMenu.js Show resolved Hide resolved
src/background/modules/ContextMenu.js Show resolved Hide resolved
src/background/modules/ContextMenu.js Outdated Show resolved Hide resolved
src/common/modules/UnicodeTransformationHandler.js Outdated Show resolved Hide resolved
@rugk rugk requested a review from tdulcet March 2, 2021 15:34
@rugk rugk requested review from tdulcet and removed request for tdulcet March 9, 2021 19:02
Copy link
Collaborator

@tdulcet tdulcet left a comment

Choose a reason for hiding this comment

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

Look good to me! Thanks again for doing this.

src/common/modules/data/Fonts.js Show resolved Hide resolved
src/manifest.json Outdated Show resolved Hide resolved
@rugk rugk requested a review from tdulcet March 15, 2021 10:53
Copy link
Collaborator

@tdulcet tdulcet left a comment

Choose a reason for hiding this comment

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

Looks good to me! Feel free to merge this whenever you are ready.

@rugk rugk merged commit 3f7cf4b into main Mar 15, 2021
@rugk rugk deleted the localisationTry2 branch March 15, 2021 13:10
@rugk rugk modified the milestones: 0.5-beta, 0.1 Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n/l10n internationalisation & localisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants