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

Docs: hosting integrations for build.commands #9755

Closed
wants to merge 15 commits into from

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 23, 2022

Initial file to start documenting hosting integrations.

This is not ready to review or merge. I'm just pushing this changes to ask for an early quick review to know if this is the direction we want to follow with this document.

Document at https://docs--9755.org.readthedocs.build/en/9755/guides/hosting-integrations.html

Related #9036
Related https://github.com/readthedocs/meta/issues/71
Related https://github.com/readthedocs/meta/discussions/35


📚 Documentation previews 📚

Initial file to start documenting hosting integrations.

Related #9036
Related readthedocs/meta#71
Related readthedocs/meta#35
@humitos humitos self-assigned this Nov 23, 2022
@humitos humitos force-pushed the humitos/build-commands-integrations branch from 3e41f22 to 3efaaa7 Compare November 23, 2022 16:14
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a great start. I think we can probably document this, but then have the goal to have the next step be to wrap all this up so you just need a single JS file that pulls the rest of that stuff in. 👍

I do think we need to figure out our plans for #9063 here as well, since we might be injecting that JS file automatically for users as part of v2 🤔

Level up your search by providing immediate feedback while the user is typing.
Read more at :doc:`advanced-search.html#enable-search-as-you-type-in-your-documentation`.

`Version warning`_
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, I thought we had an extension for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a core feature that handles the common use case and a Sphinx extension that's more configurable. However, IIRC it broke some time ago due to CORS changes.

docs/user/guides/hosting-integrations.rst Show resolved Hide resolved
Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

I know this is a draft, so I want to just leave a few general inputs :)

  • I think that this is explanation? In this regard, we should consider moving it out of the guides/ folder because that folder is mostly How-to. It would make a lot of sense if you already now suggest a folder for explanation, perhaps it's as easy as explanation/? (none of the current iterations have any proposals on where to put explanation)

  • Start the section by writing what it is about. This might be shortened later, but it's really helpful in the beginning.

  • Flyout menu: The word "Flyout" is super weird. I've put it in actual quotes in some page titles. Even what it refers to is confusing to me :D

A flyout occurs when a batter hits the ball in the air (not including balls designated as line drives) and an opposing defender catches it before it hits the ground or fence.

❓ 😕

If you happen to think of something, please say so.. already in Iteration 1 #9746 we are looking at the pages about "Flyout" menus.

You've started explaining the "Flyout" menu already in a subsection, so that might also be a reason to coordinate things closely. Otherwise, this work might be easier in Iteration 3 or 4 when we have wrestled with the existing contents and have started to focus more on developing new content.

docs/user/guides/hosting-integrations.rst Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Nov 28, 2022

@ericholscher I updated this page a little more adding more explanations for some of the other features. I added some notes to consider/discuss/talk about because telling people to "include this chunk of HTML" won't work for some features (e.g. version warning).

I think it is still fine to communicate "what we do", but I don't think it works as "integrate these features in your doctool" yet. IMO, we are not there yet and we should decouple them from being Sphinx extensions and migrate them into a Javascript library that does all this work (for the version warning example, adds the HTML, put the CSS to make it sticky at the top, generate the correct links, etc).

Writing this document is being a good exercise to realize what is required to make it fully compatible and generic enough, 💯

Flyout menu
-----------

Displying the flyout menu requires adding some Javascript and CSS files in each of the documentation pages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Displying the flyout menu requires adding some Javascript and CSS files in each of the documentation pages.
Displaying the flyout menu requires adding some JavaScript and CSS files in each of the documentation pages.

External (pull request) version warning
---------------------------------------

On each build built from a pull requests,
Copy link
Contributor

@benjaoming benjaoming Nov 28, 2022

Choose a reason for hiding this comment

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

Suggested change
On each build built from a pull requests,
For each :ref:`pull request <pull-requests:Preview Documentation from Pull Requests>`,

@ericholscher
Copy link
Member

@benjaoming Flyout is a standard term for what we're doing.. https://elementor.com/resources/glossary/what-is-flyout-navigation/

@benjaoming
Copy link
Contributor

Good reference to know. It'd still be interesting to figure out a better name. For now, I think we should resort to be careful to always mention it with a definition or reference.

To me as a reader, the word "flyout" is kind of alienating if it doesn't come with a definition or reference.

@ericholscher
Copy link
Member

ericholscher commented Nov 28, 2022

@benjaoming Yea, I think in general we try to :term: it. Agreed on finding a better name for it though. We used to say Footer, which is actively more confusing :D I think something like RTD Console or Command bar or similar could be explicit, but also confused in other ways.

@humitos
Copy link
Member Author

humitos commented Nov 29, 2022

(for the version warning example, adds the HTML, put the CSS to make it sticky at the top, generate the correct links, etc).

I'm suggesting our Javascript library should inject these things in a floating/sticky way because they don't break the HTML structure. Take a look at how Flask does this: https://flask.palletsprojects.com/en/1.0.x/

Screenshot_2022-11-29_16-47-32

@humitos
Copy link
Member Author

humitos commented Dec 12, 2022

📓 Note to myself:

If you require more detailed analytics, Read the Docs has native support for Google Analytics.

(from https://github.com/readthedocs/readthedocs.org/pull/9677/files#diff-98cc398741c4326533ef8f26b360e24f914d4ef3998e69474d10a163e42eaa4dR87)

We need to explain here how this works and where this is injected? How do we plan this to work on non-Sphinx projects?

@humitos
Copy link
Member Author

humitos commented Jan 31, 2023

This is another potentially good integration feature: #9854 and also #1802

@humitos
Copy link
Member Author

humitos commented Mar 11, 2023

I'm closing this PR for now since is superseded by #10127. The important conversation will be happening there.

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

Successfully merging this pull request may close these issues.

3 participants