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

Deep links to chapter content #7

Open
matt-gardner opened this issue Apr 25, 2019 · 6 comments
Open

Deep links to chapter content #7

matt-gardner opened this issue Apr 25, 2019 · 6 comments
Labels
enhancement New feature or request

Comments

@matt-gardner
Copy link

Hi @ines, this is awesome, and we're looking into using this to revamp our intro material for allennlp. One issue that would be great to solve is that the content inside of a chapter is not linkable. E.g., if I want to link directly to the "Loading models" exercise in chapter 1 of your spacy course, I can't do it. Is there an easy way to fix this? I tried looking into a few options myself, but I figured you would know better than me.

Similarly (and I think relatedly, which is why I bring it up here), it'd be great if clicking outside of an exercise collapsed it - I believe one way to solve both of these is to make isExpanded part of the react component's state (and set from the slug), which is reset if you click outside of an exercise, though I'm not familiar enough with react to be confident of doing this right.

@ines
Copy link
Owner

ines commented Apr 26, 2019

this is awesome, and we're looking into using this to revamp our intro material for allennlp.

Oh, cool!

One issue that would be great to solve is that the content inside of a chapter is not linkable.

Yes, I agree 👍 I actually noticed myself and wanted to fix this. The slightly annoying part here is that anchor links can be a bit annoying to handle in Gatsby and in the past, I've always had to work around them manually by tracking route changes. But I'll look into this again.

Once we have the anchor value, we should then be able to use that ID to auto-expand the exercise, yes. The nice thing about the expanded state is that we pass this in via the context in the top-level chapter and then handle the expanding in the exercise by checking if its ID matches the currently expanded ID. So at the top-level chapter, we know which exercise is expanded and at any other point in the app we can also auto-expand an exercise if you follow an anchor link that points to it. For the auto-collapsing, I guess it could check whether the click is outside of the exercise and then set setActiveExc(null).

Edit:

it'd be great if clicking outside of an exercise collapsed it

I just tried this, and it works, but... it feels kinda weird? It's okay for ther slides because they feel a bit more like a modal. But in a regular exercise, it's pretty easy to accidentally click somewhere outside of the container. And then the collapse is unexpected. Also, the exercises currently remount on expand, so if you accidentally collapse them, you'd lose your work. So I'd be reluctant to make this an "official" feature.

However, if you do want it, here's the code for src/exercise.js:

useEffect(() => {
    if (isExpanded && excRef.current) {
        excRef.current.scrollIntoView()
    }
    document.addEventListener('click', handleClickOutside, false)
    return () => document.removeEventListener('click', handleClickOutside, false)
}, [isExpanded])

const handleClickOutside = ({ target }) => {
    if (isExpanded && excRef.current && !excRef.current.contains(target)) {
        setActiveExc(null)
    }
}

Edit 2: Just pushed an update for this to the feature/deep-links branch. Was actually a simpler fix than I expected. Does this work for you?

@ines ines added the enhancement New feature or request label Apr 26, 2019
ines added a commit that referenced this issue Apr 26, 2019
@matt-gardner
Copy link
Author

Thanks, this is great! Yeah, I was thinking mostly for slides, to be able to collapse them, and it doesn't make as much sense for other exercises. I also several times hit esc trying to close the slides on your course, but reveal.js handled the esc and did weird things - maybe I'll just bind esc to close the exercise, instead of a click outside. Thanks for that.

And I tried your deep-links branch, and the links work great. The one issue is that they reset the URL to the chapter after opening the exercise, and so the history gets messed up and you can't use the back button to get back to where you were.

@matt-gardner
Copy link
Author

And for getting back to exactly where you were, having individual slides have their own URL would also be awesome, but I don't know how feasible that is (looks like reveal.js does this, but I don't know much about how gatsby works).

@matt-gardner
Copy link
Author

Hmm, and because the links to exercises have numbers instead of names, we'd have to modify links if the course content changes... That unfortunate, but probably even harder to get right than giving slides their own URL, because you rely on exercises having integer ids so that you can go to the next exercise. Maybe there's a way to also give exercises names, and use those for links...? Or make a way to get the next functionality without integers, by parsing the exercises in the chapter and creating a "next" dictionary that's keyed by id, whether or not it's an integer? You also rely on the id for the numbering on the chapter overview, but that could maybe also be done while parsing the exercises.

I also don't want to be just asking you to do a whole bunch of work for us; if something in here sounds feasible, I'm happy to try getting it to work myself.

@ines
Copy link
Owner

ines commented Apr 27, 2019

The one issue is that they reset the URL to the chapter after opening the exercise, and so the history gets messed up and you can't use the back button to get back to where you were.

Ah, yeah, that's because it's just writing to the window.location.hash. I tried replacing that with history.pushState, but that now causes a bunch of other weird behaviours. Because the slides mount differently / slower (?), I ended up having to wrap the history.pushState in a setTimeout. And the auto-expending on anchor target was still kinda flaky. I still think it's possible, but it probably needs some more thought.

Another option would be to abandon the anchors alltogether and use query parameters. It'd be less "native" (because what we're trying to do here is exactly what anchor links are supposed to be for), but it'd be more flexible, because a URL could be chapter1?exc=4&slide=2.

Hmm, and because the links to exercises have numbers instead of names, we'd have to modify links if the course content changes...

Yes, that's a good point – when I built this for my course, I didn't really consider use cases where content changes later on (because my course materials were pretty much done).

The props of the Exercise component are the attributes passed to <exercise>, so you could add an additional name that's used as the activeExc, and only use the integer id for the prev/next logic. (In the current setup, an individual exercise doesn't know its index or "who" it is – it's not rendering the exercises as a sequence and instead, just converting the full chapter from Markdown to React components. That's why you currently need to tell an exercise what its index is.)

@joelostblom
Copy link

@matt-gardner Did you end up using the solution in https://github.com/ines/course-starter-python/compare/feature/deep-links with good results? Or would you be willing to share any other modifications you have made to get links directly to sections within a chapter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants