📑 Add document outline mobile view #369
Conversation
|
@stevejpurves I've replicated the It feels like |
|
OK, I decided to make that change; I assume that as we built out the themes, it became clear that the article theme needed its own |
|
I've seen that this is a place where we have project, site, and page frontmatter. I haven't gone through these changes super carefully, so any reviewer would be welcome to cast an eye. |
|
We have 3-4 themes that are dependent on that export from site, anything in book/article cannot be exported. |
|
Hmm, OK. @rowanc1 can you nudge on whether the core site export should include this change, or whether I should fork it for the book export? |
|
I will take a more in depth look/test later this week. I forgot that we had forked the articlePage for the article theme already, so this is in family with those changes. I suppose the general advice is that exportable things need to be in the |
|
Sure, that's an idea! It might just be that the difference between the two is the addition of downloads. But I also feel that it makes sense eventually for articles and books to be "different" at the page level. |
agoose77
force-pushed
the
agoose77/wip-outline-inline
branch
from
08554a3
to
d2bb502
Compare
|
@rowanc1 I'm actually thinking that we shouldn't try and have an Are you OK with that change; dropping the |
|
@agoose77 I think that suggestion was not to have a single ArticlePage meta component that encoded both sets of changes but indeed to have two distinct page components available in That's leaves us with some naming to sort out but then provides two jumping off points for existing page patterns to be used in downstream themes...? |
There was a problem hiding this comment.
Tried this out locally and the mobile view works great, outline appears at the top as expected and the various options seem to be picked up correctly.
Just summarizing from the discussion here, though, I think we need to keep the existing ArticlePage, etc, exports from @myst-theme/site, so we don't break dependencies from other external themes. @agoose77 - if you think the correct direction is for each theme to define it's own ArticlePage, I think it's fine to leave your new book/.../ArticlePage.tsx so book theme no longer needs to import it from @myst-theme/site. But there isn't any reason to delete the old version from there (can even mark it as deprecated?).
This means we won't be breaking dependencies for external themes; we will just be setting an example of how to do things better, and they can upgrade as they see fit.
Other than that and the template option I mentioned in my other comment, all this needs is a changeset, then should be good to go!
There was a problem hiding this comment.
@agoose77 - thanks for the updates! This is all I was thinking regarding deprecation: https://github.com/executablebooks/myst-theme/blob/agoose77/wip-outline-inline/packages/site/src/pages/Article.tsx#L45
Should all be good to go now, as far as I'm concerned.
|
Are you comfortable merging this? Or would you like a separate review e.g. from @stevejpurves? |
Fixes #363 by moving the document outline to the main body of the page.
types.tsfile that declares the template option interface.ArticlePagefrom@myst-theme/sitetothemes/book(to mirror the one already copied tothemes/article.hide_outlineandoutline_maxdepthto thearticletheme optionsI need to understand why the
articletheme has its own implementation ofArticlePage, instead of re-using that from@myst-theme/site. Perhaps @stevejpurves could shed some light? :)