Deep-links to book/chapter/verse in Torah scroll.

akivajgordon / tikkun.io

The online tikkun you always wanted, but never had.

https://www.tikkun.io

MIT License

44 stars 18 forks source link

Deep-links to book/chapter/verse in Torah scroll. #83

Closed pisomojadogrande closed 3 years ago

pisomojadogrande commented 3 years ago

Enables bookmarking and possible future links from e.g. Hebcal.

akivajgordon commented 3 years ago

Thanks for the PR! I took a quick look and it seems reasonable. I'm hoping to do a more in depth review later tonight and I'll leave some feedback.

akivajgordon commented 3 years ago

@pisomojadogrande – I just took a deeper look and have a few remarks which I'll post shortly, but just wanted to say I'm really impressed with this change. Initially, I was reluctant to pull it in before I gave it a close look, but after reviewing, it's just done so well that you've left me no choice. The test cases are really well done, and you didn't over-engineer it.

pisomojadogrande commented 3 years ago

Huh, I hadn't thought to look at that issue, but I like the proposed schema there better than what I did here. (Among other things, I have been thinking about how we could get triennial aliyah references in here - those aliyot can be on less natural boundaries than those of a full kriyah, and that's where references in an url could be really useful.)

So here's what I'm thinking: I'll rejigger this change to implement https://tikkun.io/#/r/4-9-15 to jump to that reference (במדבר ט, ט"ו).

So, kind of what you're seeing here, just some differences in the URL parsing. And I agree with your suggestion on the naming of that method, so will fix too.

Good? If you like that plan, I'll do it.

akivajgordon commented 3 years ago

Regarding the triennial aliyot, I'm not sure what direction I want to go on that yet, but feel free to keep the discussion going in #82. I agree that this change will make it easier to achieve that without needing other UI treatment.

In terms of the changes you proposed, I think that makes sense. Let's go with the #/r/x-y-z concept and see how that goes.

I think this design is "less intuitive" by looking at the URL (as in, ?book=4&chapter=9&verse=15 is to me more intuitive than #/r/4-9-15), but I don't think there are serious use-cases yet for needing to look at or edit the URL itself. Seems to me like it would be more of programmatic interface, so I could tolerate the UX concerns in favor of the flexibility at this time.

pisomojadogrande commented 3 years ago

OK, new rev, now works as https://tikkun.io/#/r/4-13-1

akivajgordon commented 3 years ago

@pisomojadogrande – looking really good. I played around with it and it works nicely as designed.

One sort of weirdness is that I can give a non-sensical ref but the resolved value isn't super intuitive (took me a few seconds to realize what happened), e.g. 6-19-99 → 1-19-1. Hard to make that leap right away. That being said, I don't think there is a clear use case for why someone would be manually using a ref that wasn't linked directly, so I don't anticipate too many such situations.

Other follow-ups that I see from this (for future work):

adding other options as mentioned in #65
syncing the hash with the navigation within the app. This will probably be how people will share deep links (i.e. navigate to parsha, expect the address bar to sync up with that, copy-paste from address bar, share).
analytics? I'd like to get an idea of which ones are most used/requested

Thanks for the great effort!