-
Notifications
You must be signed in to change notification settings - Fork 330
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
refactor badge.displayBrowserActionBadge to be used in messageHandler fixes #973 & refactoring pop.js to fixes #1513 #1523
refactor badge.displayBrowserActionBadge to be used in messageHandler fixes #973 & refactoring pop.js to fixes #1513 #1523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to run the linter. It will check the code and come back with errors for you to fix from the code you've added.
$ npm run lint
Resolve submission Validation Warnings fixes #1513 #Outreachy2019/2020 Applicant
Okay, I will do that
On Oct 10, 2019 17:31, "Maxx Crawford" <[email protected]> wrote:
@Aishat-Akinyemi <https://github.com/Aishat-Akinyemi> Be sure to run the
linter. It will check the code and come back with errors for you to fix
from the code you've added.
$ npm run lint
It's causing the build to fail
<https://travis-ci.org/mozilla/multi-account-containers/builds/596181518?utm_source=github_status&utm_medium=notification>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1523?email_source=notifications&email_token=AMUDKWEXT5M47IUHICWXYNLQN5KGVA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA47AHA#issuecomment-540667932>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUDKWH7YRMPV4WSYU5Z7U3QN5KGVANCNFSM4I7OY57A>
.
|
Thanks for the feedback
On Oct 10, 2019 20:58, wrote:
Thanks for the feedback
On Oct 10, 2019 17:31, "Maxx Crawford" <[email protected]> wrote:
@Aishat-Akinyemi <https://github.com/Aishat-Akinyemi> Be sure to run the
linter. It will check the code and come back with errors for you to fix
from the code you've added.
$ npm run lint
It's causing the build to fail
<https://travis-ci.org/mozilla/multi-account-containers/builds/596181518?utm_source=github_status&utm_medium=notification>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1523?email_source=notifications&email_token=AMUDKWEXT5M47IUHICWXYNLQN5KGVA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA47AHA#issuecomment-540667932>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUDKWH7YRMPV4WSYU5Z7U3QN5KGVANCNFSM4I7OY57A>
.
|
@maxx Crawford <[email protected]> Hello, I have effected the changes
yesterday and I have included commit that fixes issue #1513
…On Thu, Oct 10, 2019 at 5:32 PM Maxx Crawford ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Be sure to run the linter. It will check the code and come back with
errors for you to fix from the code you've added.
$ npm run lint
It's causing the build to fail
<https://travis-ci.org/mozilla/multi-account-containers/builds/596181518?utm_source=github_status&utm_medium=notification>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1523?email_source=notifications&email_token=AMUDKWDU27XLSZAK2Q5YA73QN5KIRA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHSLDWI#pullrequestreview-300200409>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUDKWD2YCWCOKVCBK5BYMTQN5KIRANCNFSM4I7OY57A>
.
|
Oh dang, this change has some conflicting changes now. @Aishat-Akinyemi - can you resolve them? |
src/js/background/messageHandler.js
Outdated
@@ -187,7 +181,7 @@ const messageHandler = { | |||
// https://bugzil.la/1314674 | |||
// https://github.com/mozilla/testpilot-containers/issues/608 | |||
// ... so re-call displayBrowserActionBadge on window changes | |||
badge.displayBrowserActionBadge(); | |||
badge.displayBrowserActionBadge("remove"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This action is actually "showVersionIndicator". We (ab)use it to get around https://bugzilla.mozilla.org/show_bug.cgi?id=1314674 but it looks like that bug is fixed now? So we can just remove this call now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@groovecoder okay, so I should only focus on refactoring badge.displayBrowserActionBadge to handle
- when the user opens the 100th container and;
- line 5 in badge.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that sounds right. And line 5 in badge.js is the "showVersionIndicator" action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, I have effected the changes
src/js/background/badge.js
Outdated
browser.browserAction.setBadgeBackgroundColor({color: color}); | ||
browser.browserAction.setBadgeText({text: text}); | ||
} | ||
if(action==="remove") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this action to showVersionIndicator
and then update the call on line 5 to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@groovecoder thanks for the feedback. Please tell me what is meant by showVersionIndicator
do you mean I should change the parameter action
to showVersionIndicator
?
Does the showversionindicator: get the version of the browser tab (in line 5 that will be incognito and in the messageHandler.js, that will be the 100th container tab)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, change it to action==="showVersionIndicator"
and on line 5, change the call to pass "showVersionIndicator"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, I have effected the changes
@@ -610,7 +610,15 @@ Logic.registerPanel(P_CONTAINERS_LIST, { | |||
const siteSettings = await Logic.getAssignment(currentTab); | |||
this.setupAssignmentCheckbox(siteSettings, currentTabUserContextId); | |||
const currentPage = document.getElementById("current-page"); | |||
currentPage.innerHTML = escaped`<span class="page-title truncate-text">${currentTab.title}</span>`; | |||
//using DOMParser to modify innerHTML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonathanKingston - is DOMParser
the best replacement for innerHTML
assignment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I create a separate PR for the issues, I thought I could submit them both but since you are still assessing whether DOMParser is the best replacement what would you advise that I do?
I also think that it would make them count as 2 separate contributions on Outreachy.
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't ever be used, it's not any safer than innerHTML
in fact it breaks the linting which captures when we use escaped
. I don't think this is a good idea at all.
Thanks for the guidance. I'll do that
…On Oct 17, 2019 15:29, "luke crouch" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/js/background/badge.js
<#1523 (comment)>
:
> const extensionInfo = await backgroundLogic.getExtensionInfo();
- const storage = await browser.storage.local.get({browserActionBadgesClicked: []});
-
- if (MAJOR_VERSIONS.indexOf(extensionInfo.version) > -1 &&
- storage.browserActionBadgesClicked.indexOf(extensionInfo.version) < 0) {
- browser.browserAction.setBadgeBackgroundColor({color: "rgba(0,217,0,255)"});
- browser.browserAction.setBadgeText({text: "NEW"});
+ function changeBadgeColorText(color, text){
+ browser.browserAction.setBadgeBackgroundColor({color: color});
+ browser.browserAction.setBadgeText({text: text});
+ }
+ if(action==="remove") {
Here, change it to action==="showVersionIndicator" and on line 5, change
the call to pass "showVersionIndicator"
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1523?email_source=notifications&email_token=AMUDKWCLMQ4SZFOGQXL7QUDQPBZEVA5CNFSM4I7OY57KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIKAF7Y#discussion_r336039282>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUDKWCAYNUBF4UUNNKNGOLQPBZEVANCNFSM4I7OY57A>
.
|
const currentWindow = await browser.windows.getCurrent(); | ||
this.displayBrowserActionBadge(currentWindow.incognito); | ||
const showVersionIndicator = await browser.windows.getCurrent(); | ||
this.displayBrowserActionBadge(showVersionIndicator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to pass the string "showVersionIndicator"
here, and you don't need the currentWindow
variable anymore, because it was only used to pass the incognito
property value, which isn't needed anymore. So you can just replace these 2 lines with:
this.displayBrowserActionBadge("showVersionIndicator");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the DOMParser changes as it reduces the ability to capture security issues.
Looks like a new PR was submitted for this, so I will close, but let me know I am misunderstanding. |
#outreachy applicant fixes #973 and #1513
I refactored badge.displayBrowserActionBadge to be used in messageHandler. However my various attempts to reuse it in pop.js resulted in error, as the 'browser Action Badge' when clicked did not display anything.
Test
I set the number of container tabs that's required to show 'achievement' to 5 (I have changed this back to hundred after testing). The tests were successful.
Problem reusing in pop.js
The picture below is how I attempted to make badge.displayBrowserActionBadge to be used in pop.js that resulted in unexpected behaviour, kindly review and advise me on what the bugs could be, thank you. I have removed this code below from the pull request