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

New theme? #1182

Open
wants to merge 68 commits into
base: master
Choose a base branch
from
Open

New theme? #1182

wants to merge 68 commits into from

Conversation

antoinem
Copy link
Contributor

Hello,

We’ve started a new theme derived from Darkfish.
You can see how it looks here:
https://basesecrete.github.io/rdoc/

As you can see, there are already quite a few changes (navigation, typography, colors, dark mode…).
We wanted to check if there was some interest, before we spend more time on it.

What do you think?

Cheers,
Antoine and Alexis

@st0012
Copy link
Member

st0012 commented Sep 30, 2024

Thank you for proposing this 🙌
I'm open to accept new optional themes and here's my thought on this topic:

  • To encourage more usage on this tool and documentation writing in Ruby in general, more theme options are needed (2~3 in rdoc may be the best?)
  • While whether rdoc maintainers will be able to maintain them long-term is surely a concern, I think it's a risk we need to take to grow the community. It can also be mitigated a bit through clear communication (e.g. we'll do our best but only the default theme is officially supported)

WDYT? @colby-swandale

A few notes to this PR:

  • Please submit document changes (e.g. update heading) in a separate PR
  • The addition of super_classes should be extracted too
  • Since it's a new theme, I think it's better to remove some references to darkfish like "Darkfish" Rdoc CSS.
  • I'm not familiar with js best practices, but my understanding is that darkfish.js unlikely to follow much of them. So I'd love to see new theme implementations not following its pattern unless necessary. I know it may be already on your roadmap, but I want to explicitly point it out.

alexisbernard added a commit to BaseSecrete/rdoc that referenced this pull request Sep 30, 2024
The purpose is to list all class ancestors in a new theme (see PR ruby#1182).
@alexisbernard
Copy link
Contributor

Thanks for the feedback. I have extracted super_classes into its own PR.

I'm not familiar with js best practices, but my understanding is that darkfish.js unlikely to follow much of them. So I'd love to see new theme implementations not following its pattern unless necessary. I know it may be already on your roadmap, but I want to explicitly point it out.

We don't have specific plan for the JS. I think the less JS it contains, the better it is. We only added few lines of JS compared to Darkfish. Otherwise it could share the JS with Darkfish, but it would be more fragile.

@colby-swandale
Copy link
Member

This template ticks several boxes of things people would like to see in darkhorse, particularly Table of Contents navigation & Dark Mode, the new Rails documentation site shows this off well. Introducing a new HTML template would add quite a significant load to maintain, so I would more like to see us pluck these features into darkhorse.

@antoinem
Copy link
Contributor Author

antoinem commented Oct 7, 2024

I've deleted the new theme folder and copied the changes in the darkfish theme folder.

@@ -1,23 +1,19 @@
<body id="top" role="document" class="file">
<%= render '_sidebar_toggle.rhtml' %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we changing darkfish templates too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Colby said:

so I would more like to see us pluck these features into darkhorse.

So I deleted the new theme folder and modified the Darkfish templates.
Here, the sidebar toggle is rendered in the new topbar partial.
I would have preferred to keep it separated and fine-tune it until it eventually becomes the new default theme, but I understand adding a new theme would require more maintenance.
How would you like us to proceed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed that comment. I strongly prefer having it as a new theme and maintain it in parallel at least for a while.
But given that Colby and I have contrary opinions now, I'd advise you to hold off any future changes until we reached an agreement 🙏

@st0012
Copy link
Member

st0012 commented Oct 7, 2024

Introducing a new HTML template would add quite a significant load to maintain, so I would more like to see us pluck these features into darkhorse.

@colby-swandale It's darkfish, not darkhorse 🙂
While I agree it'll be hard to maintain, I don't think it's a good idea to suddenly accept a theme overhaul when it'll be directly synced to docs.ruby-lang.org. I strongly prefer having this PR just add a new theme, potentially marking it as experimental, then decide what to do next depends on how rorvswild and darkfish are updated later.

@simi
Copy link

simi commented Oct 7, 2024

IMHO good idea to keep it separate as well for some time and switch defaults at some point with major bump or so.

@colby-swandale
Copy link
Member

colby-swandale commented Oct 7, 2024

Apologies, my suggestion has gotten lost in translation I think. I wasn't suggesting that the entire Darkfish template be replaced, Instead I was suggesting plucking specific features from this Pull Requests into new PRs that can be added to darkfish like Table of Contents, Dark mode etc. Of course any new changes to Darkfish will need to consider the existing theme and make suitable adjustments.

@miguno
Copy link

miguno commented Oct 15, 2024

+1

This is IMHO a significant improvement over the status quo. Thanks a lot for your work!

@alexisbernard
Copy link
Contributor

We moved the theme into it's own gem (https://github.com/BaseSecrete/rorvswild-theme-rdoc), so anyone can use it without waiting for merging.

If you would like to have it directly into RDoc, we will be very happy to update the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants