search

glam-lab / degender-the-web

A Chrome extension that replaces all gendered pronouns with "they/them/their."
GNU Affero General Public License v3.0
6 stars 3 forks source link

Grey out the extension icon when pronoun replacement is being prevented #113

Closed beszel closed 5 years ago

beszel commented 5 years ago

Resolves #96

ProfJanetDavis commented 5 years ago

I agree this works correctly, but the commit history for this PR is difficult to make sense of. In particular, I note that "Run full Puppeteer #101" appears twice in the commit history, and I bet a bunch of other commits do as well. It looks like the duplicate commits are all from the popup branch.

I don't want to introduce duplicate commits to the master branch, if we can help it. I think you should close this PR, cherry pick the commits relevant to this feature into a new branch based on master, and make a new PR from the new branch.

Does this make sense to you? Or should we meet and figure it out together?

ProfJanetDavis commented 5 years ago

On thinking it over, I tried this at the console:

git checkout master
git merge --squash grey-icon

The actual changes are in few enough files I'm okay with a squash merge. That means it's not necessary to clean up the commit history in this branch.

I'm a bit surprised not to see end-to-end tests of this feature. Is this still work in progress? Or did you decide that testing isn't feasible?

beszel commented 5 years ago

All the duplicate commits are actually left from branching off of popup (#109), and you'll see duplicate commits there too. I then asked you and Professor Stratton about how to prevent that when rebasing. I didn't make a big deal of it since we're squashing anyway.

I think I assumed that end-to-end testing isn't feasible here, but I didn't look into it! My reasoning was that the icon isn't part of any DOM that Puppeteer can access. I'll look into it and see what I can do.

beszel commented 5 years ago

I see that #109 wasn't squashed, so master now has duplicate commits. I wish I had said something while it was in review!

ProfJanetDavis commented 5 years ago

Whoops, I wish I had noticed that too! I may look into whether we can do something about that. Live and learn...

On Tue, Nov 19, 2019 at 8:40 AM Ian Hawkins notifications@github.com wrote:

I see that #109 https://github.com/glam-lab/degender-the-web/pull/109 wasn't squashed, so master now has duplicate commits. I wish I had said something while it was in review!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/glam-lab/degender-the-web/pull/113?email_source=notifications&email_token=AEXJIU5YBKAPVNVPNNFISVLQUQJGDA5CNFSM4JNTL6A2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEO3MGA#issuecomment-555595288, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXJIU5E6GTGDAOAG2RMZODQUQJGDANCNFSM4JNTL6AQ .

-- Janet Davis (she/her or they/them) Associate Professor and Microsoft Chair of Computer Science, Whitman College 509-527-5758, http://cs.whitman.edu/~davisj/ Schedule an appointment: http://meetme.so/JanetDavis

beszel commented 5 years ago

It looks like there's no way to check which version of the icon the user is seeing on a particular page, so end-to-end tests on this feature aren't feasible. The closest thing is the existing tests for popup status messages.

I don't think anything needs to be added to the manual test page.

For manual testing as part of this code review, I reccommend this comment to see how the icon should behave on different pages.

ProfJanetDavis commented 5 years ago

Thanks, Ian, this makes sense. For future reference, can you add the gist of that comment to the manual test page?

On Sat, Nov 23, 2019 at 4:56 PM Ian Hawkins notifications@github.com wrote:

It looks like there's no way to check which version of the icon the user is seeing on a particular page, so end-to-end tests on this feature aren't feasible. The closest thing is the existing tests for popup status messages.

I don't think anything needs to be added to the manual test page.

For manual testing as part of this code review, I reccommend this comment https://github.com/glam-lab/degender-the-web/issues/96#issuecomment-551963448 to see how the icon should behave on different pages.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/glam-lab/degender-the-web/pull/113?email_source=notifications&email_token=AEXJIU75W25BUXGCSIAJH3DQVHGLLA5CNFSM4JNTL6A2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFABESA#issuecomment-557847112, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXJIU6NOTXE2DGJSCWMLU3QVHGLLANCNFSM4JNTL6AQ .

-- Janet Davis (she/her or they/them) Associate Professor and Microsoft Chair of Computer Science, Whitman College 509-527-5758, http://cs.whitman.edu/~davisj/ Schedule an appointment: http://meetme.so/JanetDavis

ProfJanetDavis commented 5 years ago

Cleaning up master and rebasing took care of the duplicate commits here. I am making additional notes on the manual tests page per your comments on #96.

ProfJanetDavis commented 5 years ago

Merged from the command line.