Skip to content

Allow listing outside URLs in extras #2103

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

Merged
merged 16 commits into from
Apr 26, 2025

Conversation

sorentwo
Copy link
Contributor

@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
Contributor 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
Contributor Author

sorentwo commented Apr 7, 2025

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

@@ -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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-04-15 at 15 02 26

How is that?

@sorentwo
Copy link
Contributor Author

@josevalim Suggestions and changes made, ready for another pass

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I have only two tiny comments remaining and we can ship it!

@sorentwo
Copy link
Contributor Author

@josevalim Updated!

@josevalim josevalim closed this Apr 26, 2025
@josevalim josevalim reopened this Apr 26, 2025
@josevalim
Copy link
Member

Closing and reopening to kick CI back to life.

Copy link

github-actions bot commented Apr 26, 2025

@josevalim josevalim merged commit 15f3a59 into elixir-lang:main Apr 26, 2025
5 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@starbelly
Copy link
Contributor

Hey @josevalim there was hard breaking change introduced here. That looks quite intentional. However, ex_doc does usually give a heads up in the form of a soft deprecation, although ex_doc is not obliged to per it still being 0.x. I would just like confirmation on this so I can make an informed decision about what to do in rebar3_ex_doc 😄

@josevalim
Copy link
Member

What was the breaking change? I would be glad to revert it to something less breaking.

@starbelly
Copy link
Contributor

starbelly commented May 15, 2025

What was the breaking change? I would be glad to revert it to something less breaking.

I missed some context. I think this is just a bug that got introduced here : https://github.com/elixir-lang/ex_doc/pull/2103/files#diff-126f42fa83c5dbc502baeb9fce0945b4089753ce8fe10ec03df01f6ef761a3d5R614

Sorry, this was not a breaking change in ex_doc, rebar ex_doc plugin is at fault here. TL;DR we allow pasing in a map, and it used to be it just happened to work because there was no explicit usage of Keyword, however, extras has always been documented as being a keyword list.

There's changes to make over there, not here 😄 Sorry for the noise.

@josevalim
Copy link
Member

No worries at all, if there is something we can do on our side, please let us know!

@starbelly
Copy link
Contributor

No worries at all, if there is something we can do on our side, please let us know!

Short of opting to use maps instead of keyword lists, I don't think there is much to do on this side 😄

@josevalim
Copy link
Member

I believe we can accept both keyword lists and atoms and normalize them accordingly. My goal in the short term future is to convert them to structs instead of passing around raw maps.

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
3 participants