Tab swiping #1656

StarScape · 2024-03-07T21:24:56Z

I haven't actually tested this on a phone yet (just used FF's touch emulation), so I want to make sure and do that before this actually gets merged, but this should be good enough to take a look at.

Swiping behavior implemented
Click to open copy overlay behavior is preserved
Respects tabs that are currently disabled and will not swipe to them

Couple comments/questions below.

Also, there was a test failing on my machine when I ran yarn test:firefox, but looks like that predates my commits.

src/content/popup/popup.ts

birtles

Looks great. Thanks so much for this.

I assume you'll test on iPhone? If so, I can try to test it out on my Android phone.

Unfortunately I'm on parental leave of sorts at the moment so it might take me a bit longer than usual to be able to test it.

Thanks again.

src/content/popup/popup.ts

StarScape · 2024-03-08T15:57:42Z

I assume you'll test on iPhone? If so, I can try to test it out on my Android phone.

Yup, that sounds good. I'll drop a comment here when I've had a chance to test on iPhone.

Unfortunately I'm on parental leave of sorts at the moment so it might take me a bit longer than usual to be able to test it.

That doesn't sound unfortunate to me 🙂. Enjoy your time with your kid(s), no rush at all on my end.

birtles · 2024-03-09T11:15:25Z

I assume you'll test on iPhone? If so, I can try to test it out on my Android phone.

Yup, that sounds good. I'll drop a comment here when I've had a chance to test on iPhone.

Great, thank you.

Unfortunately I'm on parental leave of sorts at the moment so it might take me a bit longer than usual to be able to test it.

That doesn't sound unfortunate to me 🙂. Enjoy your time with your kid(s), no rush at all on my end.

Thanks! Yeah, you're right. Not unfortunate at all! Thanks for your understanding!

StarScape · 2024-03-17T20:25:35Z

Hey @birtles, I've gotten around to testing this, but while I can get it to run on the iOS simulator, when I try to run on my actual iPhone (really just wanted to test that the swipe behavior feels right on a touch screen) I get provisioning/signing errors:

I'm a bit ignorant of how signing works in the Apple/iOS ecosystem, but I'm guessing the H3RB[...] identifier under "Team" is whatever you have set up under your account. If I try to change "Team" to my own name, I can get through the build setting, but then get this on install.

Has the iOS app/extension ever been set up to install from source on any Apple ID except yours? Or, is there a way I can just skip the signing step altogether to run the development version? Again, sorry for my ignorance of Xcode here.

birtles · 2024-03-18T02:51:14Z

Hey @birtles, I've gotten around to testing this, but while I can get it to run on the iOS simulator, when I try to run on my actual iPhone (really just wanted to test that the swipe behavior feels right on a touch screen) I get provisioning/signing errors:

Thanks @StarScape! I got in to the office for a brief moment last week where I was able to test the PR on my Android phone but unfortunately the swiping didn't work for me then. I haven't had a chance to debug it yet, however.

I'm really unfamiliar with XCode but I believe @shirakaba has set it up to work on his phone before? Jamie, do you know how any of this works?

shirakaba · 2024-03-18T09:56:28Z

Heya!

Has the iOS app/extension ever been set up to install from source on any Apple ID except yours? Or, is there a way I can just skip the signing step altogether to run the development version? Again, sorry for my ignorance of Xcode here.

Should be solvable – the Xcode project is not tied to any particular device, but it does default to signing for Brian's team (which is unavoidable – Xcode projects are very clunky for opensource projects), so you'll just need to override the team name to your personal team.

If I try to change "Team" to my own name, I can get through the build setting, but then get this on install.

As mentioned, it's correct to change the team to your personal team while working on it. 👍 Do leave the resulting change to the Xcodeproj uncommitted, though.

As for the error, could you try the top suggestion here? Happy to help until you're unblocked, so just give me another poke if that doesn't resolve it.

StarScape · 2024-03-18T16:40:49Z

@shirakaba Thanks for the guidance! That was an easier fix than I thought (though maybe 'uninstall and reinstall it' should have been more obvious 😅). I was able to get it working and tested on my phone.

it does default to signing for Brian's team (which is unavoidable – Xcode projects are very clunky for opensource projects

Yeah, it seemed a bit clunky, that's why I wanted to check here. Wasn't sure if it was an Xcode limitation or if I had misconfigured something on my end. Thanks again.

@birtles As for the swiping not working on your Android phone, I think the issue might have been that I had the x-distance that needed to be traveled in order for the swipe to register was rather high. When I tested on my iPhone, I could get it to happen, but only if I did a very long and deliberate swipe.

I've knocked the threshold down significantly now. Give it another shot when you get the chance. If it still doesn't work then let me know, we'll need to do some more serious debugging. I don't have a physical Android device handy at the moment, but I should be able to fix any bugs with an emulator.

yarn.lock

birtles · 2024-03-19T06:49:53Z

@shirakaba Thank you so much!

@birtles As for the swiping not working on your Android phone, I think the issue might have been that I had the x-distance that needed to be traveled in order for the swipe to register was rather high. When I tested on my iPhone, I could get it to happen, but only if I did a very long and deliberate swipe.

I've knocked the threshold down significantly now. Give it another shot when you get the chance. If it still doesn't work then let me know, we'll need to do some more serious debugging. I don't have a physical Android device handy at the moment, but I should be able to fix any bugs with an emulator.

@StarScape Great, that could well be it. I'll try again on Thursday.

birtles

I tried this on my Android phone and it works great. Thank you!

src/content/content.ts

src/content/popup/popup.ts

src/content/popup/swipe.ts

StarScape · 2024-03-21T17:34:19Z

Alright @birtles, it should (hopefully) be good to go now. Let me know if you'd prefer me squash these down into one commit.

birtles

Looks great. I left a few nit comments. If you don't mind, I'll just make those tweaks myself then squash merge it.

birtles · 2024-03-22T07:24:08Z

src/content/popup/popup.ts


-  const contentContainer = html('div', { class: 'content' });
+    console.log('Hello1');


Can we drop this? 😅

Suggested change

console.log('Hello1');

Lol yeah, sorry that slipped through from some debugging. Thanks for getting this merged, appreciate the patience with the back and forth!

Not at all! Thank you so much for your patience with all my nitpicking!
I'll probably try to get in a few more fixes and then we can release the next version with this included.

birtles · 2024-03-22T07:24:25Z

src/content/popup/popup.ts

-  const contentContainer = html('div', { class: 'content' });
+    console.log('Hello1');
+    onHorizontalSwipe(contentContainer, (direction) => {
+      console.log('Hello?');


And this?

Suggested change

console.log('Hello?');

birtles · 2024-03-22T07:24:59Z

src/content/popup/popup.ts

+      if (options.onSwitchDictionary) {
+        options.onSwitchDictionary(direction === 'left' ? 'prev' : 'next');
+      }


I think we can just write:

Suggested change

if (options.onSwitchDictionary) {

options.onSwitchDictionary(direction === 'left' ? 'prev' : 'next');

}

options.onSwitchDictionary?.(direction === 'left' ? 'prev' : 'next');

Fixes birchill#1653

- Change x-threshold for swipe motion from 150 to 50px - Move swipe detection code to its own file - Allow 'prev' parameter for showDictionary function - Add @stylistic/eslint-plugin-js to handle max-len linting for comments

birtles · 2024-03-22T07:47:23Z

@all-contributors please add @StarScape for code

allcontributors · 2024-03-22T07:47:32Z

@birtles

I've put up a pull request to add @StarScape! 🎉

StarScape commented Mar 7, 2024

View reviewed changes

src/content/popup/popup.ts Outdated Show resolved Hide resolved

StarScape commented Mar 7, 2024

View reviewed changes

src/content/popup/popup.ts Outdated Show resolved Hide resolved

birtles reviewed Mar 8, 2024

View reviewed changes

src/content/popup/popup.ts Outdated Show resolved Hide resolved

src/content/popup/popup.ts Outdated Show resolved Hide resolved

src/content/popup/popup.ts Outdated Show resolved Hide resolved

shirakaba reviewed Mar 19, 2024

View reviewed changes

yarn.lock Outdated Show resolved Hide resolved

birtles approved these changes Mar 21, 2024

View reviewed changes

StarScape added a commit to StarScape/10ten-ja-reader that referenced this pull request Mar 21, 2024

Minor changes from feedback on PR birchill#1656

8902dae

birtles approved these changes Mar 22, 2024

View reviewed changes

StarScape and others added 7 commits March 22, 2024 16:29

chore: fix broken link in README.md

e2592ad

feat: Allow swiping left/right between tabs in popup

69c13ac

Fixes birchill#1653

Improvements from PR feedback and device testing

1dae737

- Change x-threshold for swipe motion from 150 to 50px - Move swipe detection code to its own file - Allow 'prev' parameter for showDictionary function - Add @stylistic/eslint-plugin-js to handle max-len linting for comments

Remove ESLint max-len rule and stylistic plugin

b60d45e

Minor changes from feedback on PR birchill#1656

ac68c44

Address minor nits

f72cd82

Update CHANGELOG.md

ea60664

birtles force-pushed the tab-swiping branch from 8902dae to ea60664 Compare March 22, 2024 07:39

birtles enabled auto-merge (squash) March 22, 2024 07:41

birtles merged commit a534b29 into birchill:main Mar 22, 2024
1 check passed

allcontributors bot mentioned this pull request Mar 22, 2024

add StarScape as a contributor for code #1684

Merged

birtles mentioned this pull request Mar 23, 2024

Allow swiping between tabs #1653

Closed

StarScape mentioned this pull request May 17, 2024

Update contribution guide with extra info about Xcode #1772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tab swiping #1656

Tab swiping #1656

StarScape commented Mar 7, 2024 •

edited

Loading

birtles left a comment

StarScape commented Mar 8, 2024 •

edited

Loading

birtles commented Mar 9, 2024

StarScape commented Mar 17, 2024

birtles commented Mar 18, 2024

shirakaba commented Mar 18, 2024 •

edited

Loading

StarScape commented Mar 18, 2024

birtles commented Mar 19, 2024

birtles left a comment

StarScape commented Mar 21, 2024

birtles left a comment

birtles Mar 22, 2024

StarScape Mar 23, 2024

birtles Mar 23, 2024

birtles Mar 22, 2024

birtles Mar 22, 2024

birtles commented Mar 22, 2024

allcontributors bot commented Mar 22, 2024


		const contentContainer = html('div', { class: 'content' });
		console.log('Hello1');

Tab swiping #1656

Tab swiping #1656

Conversation

StarScape commented Mar 7, 2024 • edited Loading

birtles left a comment

Choose a reason for hiding this comment

StarScape commented Mar 8, 2024 • edited Loading

birtles commented Mar 9, 2024

StarScape commented Mar 17, 2024

birtles commented Mar 18, 2024

shirakaba commented Mar 18, 2024 • edited Loading

StarScape commented Mar 18, 2024

birtles commented Mar 19, 2024

birtles left a comment

Choose a reason for hiding this comment

StarScape commented Mar 21, 2024

birtles left a comment

Choose a reason for hiding this comment

birtles Mar 22, 2024

Choose a reason for hiding this comment

StarScape Mar 23, 2024

Choose a reason for hiding this comment

birtles Mar 23, 2024

Choose a reason for hiding this comment

birtles Mar 22, 2024

Choose a reason for hiding this comment

birtles Mar 22, 2024

Choose a reason for hiding this comment

birtles commented Mar 22, 2024

allcontributors bot commented Mar 22, 2024

StarScape commented Mar 7, 2024 •

edited

Loading

StarScape commented Mar 8, 2024 •

edited

Loading

shirakaba commented Mar 18, 2024 •

edited

Loading