Skip to content
This repository has been archived by the owner. It is now read-only.

Show Locale Strings on Extensions Page #7879

Merged
merged 1 commit into from Apr 3, 2017
Merged

Show Locale Strings on Extensions Page #7879

merged 1 commit into from Apr 3, 2017

Conversation

@jonathansampson
Copy link
Collaborator

jonathansampson commented Mar 25, 2017

Test Plan

  1. Enable Dashlane
  2. Navigate to about:extensions
  3. Confirm that Dashlane's name and description are shown

Description

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fixes #7826 #7683

image

@jonathansampson jonathansampson added this to the 0.14.1 milestone Mar 25, 2017
@jonathansampson jonathansampson requested a review from bsclifton Mar 25, 2017
@jonathansampson jonathansampson changed the title Ext locale strings Show Locale Strings on Extensions Page Mar 25, 2017
@jonathansampson jonathansampson force-pushed the ext_locale-strings branch from 0545e70 to 92850ff Mar 25, 2017
@bsclifton bsclifton requested a review from cezaraugusto Mar 30, 2017
Copy link
Contributor

cezaraugusto left a comment

Looks good visually and works ok for some extensions but needs another check to avoid issues while toggling enabled/disabled extension state.

if (fs.existsSync(msgPath)) {
let messages = JSON.parse(fs.readFileSync(msgPath).toString())
properties.forEach((property) => {
let key = installInfo[property].match(pattern)[1]

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 1, 2017

Contributor

I think you need another check here. While toggling passwords I got the following error reported on console:

An uncaught exception occurred in the main process Uncaught Exception:
TypeError: Cannot read property '1' of null
    at properties.forEach (/Users/cezaraugusto/dev/browser-laptop/app/extensions.js:376:57)
    at Array.forEach (native)
    at insertLocaleStrings (/Users/cezaraugusto/dev/browser-laptop/app/extensions.js:375:20)
    at process.on (/Users/cezaraugusto/dev/browser-laptop/app/extensions.js:358:19)
    at emitOne (events.js:96:13)
    at process.emit (events.js:188:7)
    at process.on (/Users/cezaraugusto/dev/browser-laptop/node_modules/electron-prebuilt/dist/Brave.app/Contents/Resources/electron.asar/browser/api/extensions.js:37:11)
    at emitOne (events.js:96:13)
    at process.emit (events.js:188:7)

This comment has been minimized.

Copy link
@cezaraugusto

cezaraugusto Apr 1, 2017

Contributor

fyi: log error happened while toggling last pass and pocket

This comment has been minimized.

Copy link
@jonathansampson

jonathansampson Apr 2, 2017

Author Collaborator

@cezaraugusto What do you mean by toggling? Were you enabling and disabling the extension(s) in Preferences?

Nevermind. I repro.

Copy link
Member

bsclifton left a comment

Tested it out, works perfectly! Great job on this one 😄

@cezaraugusto Sampson had updated after your feedback and I manually tested, looked great. Do you mind also confirming? (will merge here)

@bsclifton bsclifton merged commit e6df719 into master Apr 3, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@bsclifton bsclifton deleted the ext_locale-strings branch Apr 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.