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

Allow listing outside URLs in extras #2103

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sorentwo
Copy link

@sorentwo sorentwo commented Mar 31, 2025

This makes it possible to delcare URLs as extras and have them listed as links in the sidebar. For example, to set a "Wikipedia" url:

extras: [
  "Wikipedia": [url: "https://wikipedia.com"]
]

This is currently light on some validations. I'll expand the tests and URI validation if the approach is improved.

  • Handle urls in epub
  • Validate the url value is an actual URI
  • Expand formatter tests
  • Document usage of url options

Closes #2084

This makes it possible to delcare URLs as extras and have them listed as
links in the sidebar. For example, to set a "Wikipedia" url:

```elixir
extras: [
  "Wikipedia": [url: "https://wikipedia.com"]
]
```

Closes elixir-lang#2084
Comment on lines -22 to -24
if custom_search_data = map[:search_data] do
extra_search_data(map, custom_search_data)
else
Copy link
Author

Choose a reason for hiding this comment

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

Added a clause for this rather than a conditional in the function body.

@josevalim
Copy link
Member

For the epub, because there is no sidebar, should we even render them? Or should we just discard them upfront? Also, I wouldn't validate the url. It is the user docs, they can put whatever they want in there. :) Especially because even something like foo.html or foo.pdf would be valid ones.

@sorentwo
Copy link
Author

sorentwo commented Apr 7, 2025

Suggestions applied, changes made , and residual TODO items addressed. This is all set!

@@ -59,6 +31,34 @@ defmodule ExDoc.Formatter.HTML.SearchData do
end)
end

defp extra(%{url: url} = map) do
[encode("#{map.id}", map.title, :extras, url)]
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 should have caught it the first time around, but I am thinking this likely should not show up on search? I.e. we should skip URLs here too.

Copy link
Member

Choose a reason for hiding this comment

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

And let's add a test for whatever decision we pick here. :)

@@ -378,6 +379,7 @@ defmodule Mix.Tasks.Docs do
title but keep the source file unchanged.
* `:search_data` - A list of terms to be indexed for autocomplete and search. If not provided, the content
of the extra page will be indexed for search. See the section below for more.
* `:url` - An external url to link to from the sidebar.
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking we could break it apart like this:

## Customizing Extras

There are two sources for extras, filenames and urls.

For filenames, the allowed configuration is:

  * ...

For urls:

  * title
  * url

@@ -112,6 +112,24 @@ defmodule ExDoc.Formatter.HTMLTest do
assert LazyHTML.text(bar_content["h1"]) == "README bar"
end

test "extras defined as external urls", %{tmp_dir: tmp_dir} = context do
config =
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a test that shows grouping work. Imagine you want all URLs in a section in the sidebar called Important Links. The Template.sidebar_extras kinda implies URLs work with groups but we need to be sure it works "end to end".

Copy link
Member

Choose a reason for hiding this comment

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

It seems groups expect the filename but there is none here, so we probably have to pass the filename or the url (and update its docs accordingly).

@@ -59,6 +59,7 @@ export function initialize () {
const items = []
const hasHeaders = Array.isArray(node.headers)
const translate = hasHeaders ? undefined : 'no'
const href = node?.url || `${node.id}.html`
Copy link
Member

Choose a reason for hiding this comment

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

Should we add an icon, such as this one, for URLs?

If so, you can upload this bundle to remixicon.com, add external link, and get the new font back: https://github.com/elixir-lang/ex_doc/blob/main/assets/fonts/RemixIconCollection.remixicon

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

Successfully merging this pull request may close these issues.

Allow urls as extra
2 participants