just-the-class icon indicating copy to clipboard operation
just-the-class copied to clipboard

better supporting just-the-class from upstream (just-the-docs)

Open mattxwang opened this issue 3 years ago • 5 comments

Hi @kevinlin1! I'm one of the new maintainers for just-the-docs. Big fan of just-the-class (and, CS Ed - your research / pedagogy sounds very cool)!

Wanted to open this issue to see if there are any things we can implement upstream to better support just-the-class; I'm wrapping up the 0.4.0 release candidate, and can slot in any changes you have in mind before the fall quarter starts! Looking to be more proactive now that just-the-docs is actively maintained again (for example, another maintainer implemented a PR to significantly improve nav generation for large sites - https://github.com/just-the-docs/just-the-docs/pull/956).

If not, no worries - feel free to close this issue!

mattxwang avatar Sep 12 '22 23:09 mattxwang

Thanks for starting this discussion! The main place for possible upstream changes would be in _sass/custom/custom.scss where I have some stylistic overrides to provide different defaults for HTML code, iframe, details, and summary.

This reminded me that I also need to backport changes from the upstream _layouts/default.html to my _layouts/minimal.html. It might be worth considering maintaining this upstream if we see the use case for a layout for special pages that should not display the side and top navigation. I use it for my primarily single-page websites, but I could also see it helpful for some special standalone pages (perhaps marketing, events, etc that aren't as connected to the primary navigation).

If this seems interesting, I could open an issue or PR upstream to work through the engineering or design considerations.

kevinlin1 avatar Sep 13 '22 00:09 kevinlin1

Thanks for the quick response Kevin!

The main place for possible upstream changes would be in _sass/custom/custom.scss where I have some stylistic overrides to provide different defaults for HTML code, iframe, details, and summary.

Gotcha - let me take a look at those and see what would make sense to bring upstream.

It might be worth considering maintaining this upstream if we see the use case for a layout for special pages that should not display the side and top navigation.

Oh, interesting! I think there's certainly a use-case for this sort of page / layout. If we make it upstream, we can likely have default consume it, which should lower code duplication on your end (as well as other users). If you have the time, feel free to open an issue and/or PR to discuss!

Independently, I'm TAing a class at UCLA this fall quarter and will give just-the-class a spin. Should make me more familiar with the theme + better understand how to support the theme moving forward! Great work with creating and maintaining it so far 😊

mattxwang avatar Sep 13 '22 20:09 mattxwang

Thanks! I just opened just-the-docs/just-the-docs#959 for discussion.

kevinlin1 avatar Sep 13 '22 22:09 kevinlin1

Just to circle back on the styles in custom.scss, I had a chance to try each of them and see which ones we can upstream. Some of them I think are easily doable, others not so much. Apologies for the massive wall of text!

massive wall of text :(

code: I think this may be too big of a stylistic change for the core of the theme :(

code {
  font-size: 14px;
  padding: 0.2em 0.4em;
  border: none;
}

details: this one is interesting. I think in general, I could see this being quite useful, but we currently rely on details having margin: 0 since it functions as a collapser for the table of contents - see the navigation structure page's TOC. I do see this being useful as a class - would adding support there be helpful?

details {
  margin: 0 40px 1em;
}

iframe/summary: this is quite reasonable to me! In general a fan of max-width: 100% default :)

iframe,
summary {
  max-width: 100%;
}

summary: for the same reason as details, this breaks the existing styling. Would a class upstream for this be helpful? Or a mixin-sort of override that is only @extend .btn, .btn-outline;?

summary {
  @extend .btn, .btn-outline;
}

.main-content-wrap: could you explain where you use this? I'm doing a visual diff on our site and I don't see any layout changes (which maybe, is a good thing!). Is this for sites that you override with the minimal layout?

.main-content-wrap {
  max-width: $content-width;
  margin: auto;
}

.main-content > dl / dt: I think the font weight and text-align overrides are probably upstreamable, though the grid-first behaviour (which was introduced a long time ago!!) is probably desired by some users. Maybe a class here?

.main-content {
  dl {
    display: block;
    grid-template-columns: none;
  }

  dt {
    font-weight: 700;
    text-align: start;

    &::after {
      content: normal;
    }
  }

  dd {
    font-weight: normal;

    + dt {
      margin-top: 1em;
    }
  }
}

upstreaming this

If you feel comfortable opening a PR, you should go for it - you did write this code! If it's easier for me to integrate these changes (I know the quarter is starting soon, so totally understandable if you don't have much time) I can take a look at doing this myself. Let me know what you think!

mattxwang avatar Sep 16 '22 03:09 mattxwang

Yeah, .main-content-wrap is needed for the minimal layouts the way I designed divs in the layout.

The grid format for definition lists (dl, dt, dd) work fine for short definition terms, but longer definition terms (phrases or sentences) didn't work well because they would push the content too far to the side. Using block format is better for these longer phrases.

kevinlin1 avatar Sep 16 '22 21:09 kevinlin1

FYI - just released rc4!

I know the new minimal layout is not a drop-in replacement for your current one, but hopefully at least you now don't need to copy over all the components. I can take a look at getting search to work as well (with your follow-up issue) - if you have bandwidth to take that on before the quarter starts though, go for it!

mattxwang avatar Jan 08 '23 01:01 mattxwang