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

Language Clarifications #1205

Open
wants to merge 9 commits into
base: 3.X.X-Branch
Choose a base branch
from
Open

Conversation

kennethtran93
Copy link
Member

@kennethtran93 kennethtran93 commented Aug 9, 2021

Resolves #754.

If anybody finds a place where the existing terms did not migrate after this version, please let me know.

@codecov
Copy link

codecov bot commented Aug 9, 2021

Codecov Report

Merging #1205 (89fb17e) into 3.X.X-Branch (4fc0f1f) will increase coverage by 1.25%.
The diff coverage is 30.64%.

@@               Coverage Diff                @@
##           3.X.X-Branch    #1205      +/-   ##
================================================
+ Coverage         47.82%   49.07%   +1.25%     
================================================
  Files                34       34              
  Lines              2731     3087     +356     
  Branches            821      972     +151     
================================================
+ Hits               1306     1515     +209     
- Misses             1424     1570     +146     
- Partials              1        2       +1     
Impacted Files Coverage Δ
src/background.ts 0.00% <0.00%> (ø)
src/redux/State.ts 100.00% <ø> (ø)
src/services/AlarmEvents.ts 40.00% <ø> (ø)
src/services/BrowserActionService.ts 70.51% <0.00%> (ø)
src/ui/common_components/ExpressionOptions.tsx 0.00% <0.00%> (ø)
src/ui/common_components/ExpressionTable.tsx 0.00% <0.00%> (ø)
src/ui/popup/App.tsx 0.00% <ø> (ø)
src/ui/popup/components/CleanCollapseGroup.tsx 0.00% <ø> (ø)
src/ui/settings/components/About.tsx 0.00% <ø> (ø)
src/ui/settings/components/Expressions.tsx 0.00% <0.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fc0f1f...89fb17e. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

ke-d
ke-d previously requested changes Aug 10, 2021
Copy link
Member

@ke-d ke-d left a comment

Choose a reason for hiding this comment

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

Hmm I'm not sure if it's necessary to have it to have Whitelist, greylist, etc. be a changeable option. Might as well just change it to Keep list, Restart list, etc. and then note the changes in the documentation. It will probably make the PR less complicated also.

@kennethtran93
Copy link
Member Author

Hmm I'm not sure if it's necessary to have it to have Whitelist, greylist, etc. be a changeable option. Might as well just change it to Keep list, Restart list, etc. and then note the changes in the documentation. It will probably make the PR less complicated also.

The issue referenced in question has seen some debate, so I'd rather just be able to provide both options. This way those that want to change can do so, and those that would like to continue using the existing terms can do so without making any changes as well.

While the PR may look like a bit much, here's what it boils down to:

  • Some accessibility enhancements to settings page (still needs work but part of it is there)
  • Extracted the editable text field from the popup/expression table to be able to use in setting area as well.
  • Added function to check if word was changed, otherwise provide default list name/type.
  • Localization changes to reflect changed words.
  • Whatever Prettier reformatted again.

@neroux
Copy link

neroux commented Aug 20, 2021

This way those that want to change can do so, and those that would like to continue using the existing terms can do so without making any changes as well.

While I fully understand where you are coming from and what you are trying to achieve, I am afraid it still is caving in to unreasonable demands, which are solely based on some political agendas and follow some fashion trend.

What if someone finds offence with cookie next? Is that going to be "customisable" as well? Will every string in the extension be eventually changeable? Where do we draw the line?

@kennethtran93
Copy link
Member Author

While I fully understand where you are coming from and what you are trying to achieve, I am afraid it still is caving in to unreasonable demands, which are solely based on some political agendas and follow some fashion trend.

I don't think this is an unreasonable demand. Sometimes change is for the better, along with an open mind.

What if someone finds offence with cookie next? Is that going to be "customisable" as well? Will every string in the extension be eventually changeable? Where do we draw the line?

I think I already answered this, but if that's the case, then the action we'll take will depend on the the changes that the web and internet browsers do as well.

@neroux
Copy link

neroux commented Aug 20, 2021

I don't think this is an unreasonable demand. Sometimes change is for the better, along with an open mind.

That's the very point, there's no improvement or change for the better.

if that's the case, then the action we'll take will depend on the the changes that the web and internet browsers do as well.

I am not aware of any changes implemented by browsers. Some services gave in to peer pressure and adjusted strings for no reason.

It's a very vocal minority who creates trouble here. The linked issue is the best example, the overwhelming majority voted against such a change, nonetheless it was implemented, when there are probably more pressing issues at hand.

@kennethtran93
Copy link
Member Author

Then we'll call it improvement and clarity of the language. If the younger/future generation doesn't already understand white and black in the tech context then at least allow and restart should be more understandable without looking them up.

@neroux
Copy link

neroux commented Aug 20, 2021

I am afraid that is a false pretence. There's no "clarity" necessary here in the first place. Aforementioned example cookie is equally misleading, there's nowhere a biscuit or cookie in sight when it comes to that unique identifier value in the header. But no changes planned here as I understand.

Yes, it is your extension and you are obviously free to name things the way you see fit, but you do ask for donations and - while I do not pretend that I am current contributor or had immediate plans to become one - I am afraid I cannot say taking such a political stance will sway me very much to consider it in the future.

As mentioned, about 80% were against that change and it was still implemented (as optional) even though - just to put it into perspective - there are five current bugs and about 40 other proper feature requests.

I apologise, but this is caving in and catering to aforementioned vocal minority with their agenda.

@AderitoSilva
Copy link

I think a good name for the lists would be “Never” (for whitelist), “On Tab Closed” (for blacklist) and “On Session Ended” (for greylist). In my perspective, that would serve the main purpose of self-explaining when the cookie automatic deletion occurs and, as a bonus, would even put an end to the black/white lists debate. On that referred issue, I explain some more details about this suggestion.

Not that I'm adhering to that debate, but I think the list concept should really be changed to something else, as it seems a bit confusing, at least to me. It seems confusing because the purpose of the lists is not clear enough, specially the greylist. And, the events' concept seems to me as a nice alternative, as it would self explain the purpose of each list. Later on, more events could be added, like an “On Domain Exited” event, which deletes cookies after we navigate out of a domain within a tab, considering the appropriate auto-deletion delay. I imagine such a change would require just visual string changes.

I suggest the option to set a visual name for each list to not be included, because it adds additional complexity into the extension settings, which doesn't benefit most users at all. In my perspective, the decision about what technical terms to use should not be postponed to users, because it consumes more of their own effort, without providing them with any additional benefit.

Regarding that specific debate, it seems to me as a fight where there are no winning sides — to please one side, you have to go against the other, and vice-versa. My experience taught me that, sometimes, the only way to not lose is to not play. I strongly recommend you to try to keep CAD development out of this kind of confrontations, as it serves a purpose that is ethically technological, but doesn't benefit from being conditioned by cultural reasons (without rational/logical motives). Sometimes, efficiency and culture go in opposite directions, where culture changes naturally while efficiency requires cognizant research. This is just a mere suggestion, based on my own personal experiences.

Keep up the good work, and thanks for this great extension! :)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kennethtran93
Copy link
Member Author

This PR has been updated to just be Language Clarification.

@kennethtran93 kennethtran93 changed the title Visual list text overrides Language Clarifications Nov 4, 2021
@github-actions

This comment has been minimized.

type: ReduxConstants.RESET_COOKIE_DELETED_COUNTER,
});
// Language Migration 3.7.0+
if (convertVersionToNumber(details.previousVersion) < 370) {
Copy link
Member

Choose a reason for hiding this comment

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

I recommend adding a test for this if you haven't already (hard to tell in this large PR), just to verify the behavior from one state to another. Another option is to split the setting migration + test into a separate PR.

src/redux/Actions.ts Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2329561540

  • 20 of 72 (27.78%) changed or added relevant lines in 9 files are covered.
  • 106 unchanged lines in 5 files lost coverage.
  • Overall coverage remained the same at 49.71%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ui/common_components/ExpressionOptions.tsx 0 1 0.0%
src/ui/settings/components/Expressions.tsx 0 1 0.0%
src/ui/common_components/ActivityTable.tsx 0 4 0.0%
src/ui/settings/components/Settings.tsx 0 4 0.0%
src/background.ts 0 16 0.0%
src/redux/Actions.ts 14 40 35.0%
Files with Coverage Reduction New Missed Lines %
src/ui/common_components/ExpressionOptions.tsx 4 0%
src/background.ts 13 0%
src/ui/settings/components/Settings.tsx 23 0%
src/redux/Actions.ts 29 32.94%
src/ui/common_components/ActivityTable.tsx 37 0%
Totals Coverage Status
Change from base Build 2288705957: 0.0%
Covered Lines: 1013
Relevant Lines: 2062

💛 - Coveralls

@github-actions
Copy link

Pull Request Build: 2329561540

Unsigned webextension builds for testing is ready for download!
These test builds are only kept for up to 90 days from the date of this post.
It is strongly recommended to use a fresh profile for testing these builds to prevent any kind of modifications to existing data.

Generating Download Links

  • /get-build for the latest commit build.
  • /get-build 2329561540 for this specific commit 89fb17e build.

When new command comment is posted, please allow a few seconds for the command to be received and processed.

  • 👀 indicates that command has been received.
  • 🚀 indicates that command has been sent for further processing.
  • 🎉 indicates that command has been executed. Expect a new comment with the result shortly.

Due to API limitations, generated links will only be valid for one minute!

@kennethtran93 kennethtran93 force-pushed the 3.X.X-Branch branch 4 times, most recently from 2e1ae20 to 659f46d Compare July 18, 2022 02:26
@kennethtran93 kennethtran93 force-pushed the 3.X.X-Branch branch 3 times, most recently from a918935 to 25332a1 Compare November 15, 2022 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename whitelist to something
5 participants