Add title attribute with mfa on links to improve seo #932

matlc · 2018-12-09T03:49:11Z

Implementation of #922

To improve the docs SEO for function discoverability, this PR adds a title attribute to links for modules, functions, etc.

Please let me know if you have any feedback.

A screenshot of dev tools showing the title:

josevalim · 2018-12-09T07:35:53Z

lib/ex_doc/nodes.ex

          source_path: nil | String.t(),
-          annotations: list()
+          annotations: list(),
+          module: nil | String.t()


Do we really need this new field? We should always know the module it belongs to based on the context. For example, for summary_entry_template, we can probably pass it when invoking it here:

ex_doc/lib/ex_doc/formatter/html/templates/summary_template.eex

Line 6 in 59905ee

<%= for node <- nodes, do: summary_entry_template(node) %>

I had thought that each node may use some info about its containing module, however, passing the module through makes sense. I'll take out the field and change the template signatures. Thanks!

eksperimental

I dont think we need the "Link to ...", We just need the name of the function, with the exception of the link chain icon where we can put "link to ..." Because we are already in the function

matlc · 2018-12-09T16:45:44Z

@eksperimental That makes sense. I'll remove the 'Link to' text except for the chain link and screenreader text. Thanks for reviewing!

eksperimental · 2018-12-09T16:47:22Z

I will leave a full review when I get to be with my computer. Thank you for taking care of this

matlc · 2018-12-16T22:52:48Z

I forgot to comment on here that the changes were implemented. Thank you all for reviewing!

milmazz

@CRYIC LGTM, just left a comment about the re-indent in the .handlebars files. Thanks for your first contribution.

milmazz · 2018-12-18T21:20:56Z

assets/js/templates/search-results.handlebars

+  </h3>
+  <ul class="functions">
+    {{#each functions}}
+    <li class="result-elem"><a href="{{../id}}.html#{{{anchor}}}" title="{{../id}}.{{anchor}}">{{{match}}}</a></li>


Is this whole re-indent actually needed for this feature? It makes a bit difficult to see the real differences unless you ignore the whitespace changes. In the future try to accommodate to the current style of send another commit just doing the re-indent.

Completely unintentional spacing changes. I neglected to undo the changes from my formatter. I'll change these and comment again once it is fixed up.

matlc · 2018-12-18T22:30:03Z

Thank you for catching that @milmazz . I undid the spacing changes on the .handlebars files. The actual changes are much easier to see.

eksperimental · 2018-12-21T02:18:32Z

I think we can be consistent with the titles in the sidebar and add them to all of the items.
For instance, where it reads "Guards" title should be "MODULE_NAME guards" (Kernel Guards),
and so on for the other items "Kernel functions", "Kernel macros", "Kernel summary".
I'm not sure about "Top", since it links to the same spot as Kernel, it we should use the title "Kernel", or "Kernel - top of the module". I am more inclined to the first one, just to be consistent with the title of the Kernel item.
As for the pages, it can have the same title as the page, and for the sections, "Compatibility between non-major Elixir versions — Deprecations" being "—" an em-dash.

While some of this may sound repetitive, I think while scrolling such longish menus, it's refreshing to mention where we are by mentioning the main module or page.

eksperimental

We should add the "link to SECTION" everywhere, not only functions,

it is not showing it for module sections and pages:
doc/elixir/api-reference.html#modules and /doc/elixir/compatibility-and-deprecations.html#compatibility-between-non-major-elixir-versions

eksperimental

when linking to functions or modules in the body of the documents, we should make use of the title.
UPDATE: this should apply for internal links and pages

eksperimental

typespecs should make use of titles:
doc/elixir/Enum.html#any?/2

<pre>any?(<a href="#t:t/0">t</a>(), (<a href="#t:element/0">element</a>() -> <a href="typespecs.html#built-in-types">as_boolean</a>(<a href="typespecs.html#built-in-types">term</a>()))) :: <a href="typespecs.html#built-in-types">boolean</a>()</pre>

should read:

<pre>any?(<a href="#t:t/0" title="Enum.t/0 type">t</a>(), (<a href="#t:element/0" title="t:Enum.element/0">element</a>() -> <a href="typespecs.html#built-in-types" title="Built-in types — Typespecs">as_boolean</a>(<a href="typespecs.html#built-in-types" title="Built-in types — Typespecs>term</a>()))) :: <a href="typespecs.html#built-in-types" title="Built-in types — Typespecs>boolean</a>()</pre>

matlc · 2019-01-02T14:00:48Z

Thanks for the thorough feedback. I'll go back through and work on these.

matlc · 2019-02-03T15:24:06Z

I'm finishing the last bit of changes and will have an update soon.

For consistency sake, should mfa signatures for typespecs be "Enum.t:element/0" or "t:Enum.element/0"?

In the sidebar, I had used the former because each node id was in the "t:function/arity" form. However, I noticed that in the typespec example code above, the mfa is the latter. Is there a proper convention?

josevalim · 2019-02-03T15:28:04Z

Awesome! Btw, “t:Enum.element/0” is the way to go as that’s also how we cross link in the docs.

eksperimental · 2019-02-03T18:31:20Z

I'm back. I will review the changes and get back to you.
Thank you.

matlc · 2019-02-03T18:37:34Z

Great! I haven’t pushed yet, but I’ll comment here again when everything is ready.

eksperimental

This is my review. Please don't feel obliged to apply everything, as those changes can go in a different PR, with the exception of point number 5, which needs to be fixed in this PR.

I'm reviewing the Atom module

doc/elixir/Atom.html

Add title to linked functions:\

See also Kernel.is_atom/1.

Title should be "Kernel.is_atom/1"

Summary link icon, should have a title "Link to Atom summary"

Link to this section Summary

This should be applied to every section in the document.

Summary > Functions
Functions title should be "Atoms functions"
to_charlist/1 > Examples
Title should read "Atom.to_charlist/1 examples"

Enum module

Title for types is wrong.
For example in doc/elixir/Enum.html#t:acc/0

It should'n t be "Link to Enum.t:acc/0",
but "Link to t:Enum.acc/0"
This should be fixed everywhere it is used MODULE.t:TYPE/ARITY
to t:MODULE.TYPE/ARITY

Within typespecs the title should be same as with the other links.
doc/elixir/Enum.html#t:acc/0

"any()" should have the title "t:any/0"

But since we are linking to the Basic type section, and not to the type itself maybe title could read "t:any/0 basic type"

Same applies for Built-in types

I think it will be helpful to add a title to the private that are not linked to anything
doc/elixir/Process.html#flag/2
"heap_size()" should have a title "private heap_size/0 type"
the same should be done with opaque types
But in this case a link does exist.

NOTE: I think adding when a type is private, opaque, or it's a built-in or basic, it helps to educate users on recognize them which are which, just by reading the title of the type in case of doubt.

Kernel module
elixir/Kernel.html#content

Add title to linked modules

Kernel is Elixir's default environment.
Kernel title should be "Kernel module"

this should also cover external Erlang modules such as:

Some of the functions described in this module are inlined by the Elixir compiler into their Erlang counterparts in the :erlang module.
where ":erlang module" title should be ":erlang module"
this is a special case, but if we call ":lists" the title should be ":lists Erlang module"

Add title to linked pages

Compatibility and Deprecations - lists compatibility between every Elixir version and Erlang/OTP, release schema; lists all deprecated functions, when they were deprecated and alternatives

"Compatibility and Deprecations" title should be "Compatibility and Deprecations page"

Thank you for your contributions

eksperimental · 2019-02-12T12:46:20Z

Hi @CRYIC
the PR got messed up because I think you merged master instead of rebasing.
My suggestion is to download master locally, rebase on top of it git rebase -i master, fix anything that needs to be fixed if needed, and the do a "forced push" by doing git push origin HEAD -f to your repo

matlc · 2019-02-12T21:21:23Z

@eksperimental Thanks for pointing that out! You're completely correct. I had merged instead of rebased master.

I'm going through your last couple of comments/suggestions to make sure I've covered them. I'll comment here with any notes.

matlc · 2019-02-12T22:41:12Z

@eksperimental From your comment #932 (review) I have accomplished number 5, but the rest are based off an earlier comment #932 (comment) .

I'll use a separate branch to get this work up to the most recent comment. Let me know if you think the current PR is okay as is.

Thanks very much for your reviewing!

matlc · 2019-03-17T23:13:17Z

As per @eksperimental 's comment above, I've gotten items 1-6 and part of 9 done here. The remaining items will need to be their own issues and/or PRs due to research and some need for insight. Just for quick reference, Items 7-8 need a bit more research in how to recognize private/opaque functions. The ':erlang' module part of 9 and 10 are difficult due to Autolink currently not linking Erlang modules or 'prelinked' links.

Concerning this PR, there a bug in Earmark that messes up Markdown parsing when two links that have titles are on the same line, reported in pragdave/earmark#220 . I think it has been fixed, however, until the release is out, it may cause problems with this work.

Let me know what you think about this PR. It covers a lot and was much larger than I'd anticipated. Thank you!

matlc · 2019-03-31T22:43:26Z

I've gotten the latest changes from master into my branch and conflicts fixed up.

eksperimental · 2019-04-03T23:17:56Z

@CRYIC I'm downloading the latest changes, I will test it and get back to you in a few days

eksperimental · 2019-04-03T23:40:48Z

@CRYIC do you think you could squash everything into one or more commits, it's really hard to review your changes as there are other changes interwoven chronologically speaking. it's 57 commits already

eksperimental · 2019-04-03T23:55:51Z

or if there is a way to run a diff and only show the changes of this PR, it will also help

matlc · 2019-04-04T19:00:03Z

@eksperimental Sorry for misunderstanding, but I'm not sure what you mean. I can definitely squash this PR into 1 commit, but I'm not sure what difference that makes to https://github.com/elixir-lang/ex_doc/pull/932/files .

Is there a different view/method you use to review? Let me know and I'll squash and push.

eksperimental · 2019-04-04T20:04:53Z

Im reviewing from my terminal. Thats why. One commit is fine for me

author Michał Łępicki <[email protected]> 1546725776 +0100 committer Mat Clutter <[email protected]> 1554419584 -0400 gpgsig -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEsM4qAF+pTkg2/b7aTIOp6Pcx4e4FAlymj4AACgkQTIOp6Pcx 4e5C4RAAr+0863jpjah1JcSKz6LBAR341VnwWSccn1ndvT8kohpFaogIqBh2luP1 iDCq6ZHNHviNoD/YrZDUfFbtcShSD70gCba5l2W2+QItQ7ncm1TGPC5iXi6wi+Cx VqrYPSedHNlkUNQEphePTj0a+7S16sZWgZZo74nKQL2jpbnEHQfQBMn3LeLVY749 M8LqePTPzpctEYqqFWuWhOM3HcWhZyZqR+eqJDbJPhIY0HwvmtL0+Hh/TzIYmBV2 UhMfQ2Xl2ZMPQsQpmdP8RuqyEE5FbUkY6eyZOw4FLPFt0gGqVlvWaUscFxzG5Jd9 B1siUUTIv9ghONyO5QlUJigAsMcFqBr27tOWW+qLsPvScR0LEneC/Fc71JvZsTsY b/CopfkzVNNNlgx9RYyp4zK00+E8aPqPUxvPLjusymcOmUTP/CxoGZjJY5+xroIp AVoL9j994ujM8Qs1IZo2gw6wXgi5NXgn6Tbu/Dy8mWDbShdqQu641K5anlya5A9S MkGYUZ2EeeHAYY8nj6HuIqmOvd23K1jz8uX0oqtDcu2T4K7PH1w81T00pwBwekhC 3CdiLbq0z2aWCfNl8O4/SapMbWIoYbZC6Z8oCrRjSzqQl1qyJQiHX7BunMpQd0zu sqjN+7s6mXxG33Qwkk3shOCLgPiF2sCvEnqF82Al3gWmphjcMEs= =m5NQ -----END PGP SIGNATURE----- Unify code styling for makeup and hljs (elixir-lang#959) Fix anchor links sometimes not working by moving js to html head (elixir-lang#962) Raise error message for unknown docs chunk Add goto icon Fix sidebar margin on Firefox Allow direct external links on the sidebar * Clicking on the sidebar now consistenty expands the menu * However, on every root link, there is now an arrow to open the page directly * If shift is pressed, we keep the browser behaviour * Improve CSS classes: clicked is now currentPage and active is now currentHash Hide formatter modules Release v0.19.3 Remove broken link Always show link Add extra padding Use the goto arrow consistently on expansions Reduce hljs FOUC to colors only (elixir-lang#960) Fix typespec arg number (elixir-lang#966) Previously signature where typespecs didn't have an argument name, were named starting with 0 time_zone_period_from_utc_iso_days(arg0, arg1) time_zone_period_from_utc_iso_days(Calendar.iso_days(), Calendar.time_zone()) :: ... this commit fixes the number starting from 1 Name args in Autolink.default_text/4 helper (elixir-lang#957) Add titles to sidebar top-right icons Improve mouseover and titles on goto, related to elixir-lang#968 Add a versions dropdown to the html sidebar (elixir-lang#970) Remove overflow on content, closes elixir-lang#972 Reduce size of app-vsn annotation Keep trailing periods in summaries (elixir-lang#977) Closes elixir-lang#975 Revert "Remove overflow on content, closes elixir-lang#972" This reverts commit 43effcb. The reverted commit breaks anchors on Firefox. Fix anchors in Firefox/Chromium Closes elixir-lang#978 Added ARIA-label to the search button Signed-off-by: José Valim <[email protected]> Add keyboard shortcuts (elixir-lang#976) Fix issues with JS events under non-webkit browsers (elixir-lang#980) Keyboard shortcuts - improvements (elixir-lang#982) Add module to node structs to carry module name; add mfa into title attribute on links to improve seo Fix tests to include title Run formatter Remove module attribute on nodes; update templates to pass in module to reference title where needed Remove 'Link to' text except for detail chain link and screenreader info Update tests to remove Link to text from api and non-detail templates Undo unintentional format of spacing Remove link to text from title attributes Update css/js with new sha version Add title attribute with mfa on links to improve seo Add module to node structs to carry module name; add mfa into title attribute on links to improve seo Remove 'Link to' text except for detail chain link and screenreader info Undo unintentional format of spacing Add title attribute with mfa on links to improve seo Update title in detail template examples Add title to replace function for links in text Update auto link to add titles to links within page content and specs Add module_id into default_title to add full mfa to local functions Lowercase section headings in titles New dist files after running build after rebase accumulation Remove rebased-in code Update module details and search results Create seperate entry function for extras headings; change casing on summary Update templates and tests to remove tags from titles Change binary block formatting keep formatter from breaking lines Full text search (elixir-lang#974) Fix required double click on sidebar for safari Improvements to full text search * Add loading state * Only load search items on search page No longer depend on Poison Move shared dependency to HTML Do not render to markdown multiple times Make the linter happy Ignore : in search as it is not stemmed anyway, closes elixir-lang#983 Provide search help on invalid syntax Remove double var Keep structs in signature, closes elixir-lang#973 List delegation docs if delegate_to metadata is present Run the formatter in released Elixir version Update CHANGELOG Search autocomplete (elixir-lang#985) Rename config.js to ex_doc_config.js Push Earmark to v1.3 (elixir-lang#988) Add module to node structs to carry module name; add mfa into title attribute on links to improve seo Run formatter Remove module attribute on nodes; update templates to pass in module to reference title where needed Remove 'Link to' text except for detail chain link and screenreader info Undo unintentional format of spacing Update css/js with new sha version Add title attribute with mfa on links to improve seo Add module to node structs to carry module name; add mfa into title attribute on links to improve seo Remove 'Link to' text except for detail chain link and screenreader info Undo unintentional format of spacing Add title attribute with mfa on links to improve seo Add module_id into default_title to add full mfa to local functions Lowercase section headings in titles New dist files after running build after rebase accumulation Remove rebased-in code Update module details and search results Create seperate entry function for extras headings; change casing on summary Update templates and tests to remove tags from titles Change binary block formatting keep formatter from breaking lines Add link_title to new searchItems list Update earmark after deps.get Update css/js assets after mix build Update css/js assets after mix build

matlc · 2019-04-04T23:47:52Z

@eksperimental A couple of things got mixed up during the squash. I'll fix those up, push, then @ you. Sorry for the trouble.

matlc · 2019-04-05T00:14:41Z

@eksperimental Okay, I think I got everything fixed up. However, the TravisCI build says failed, but, it looks like all the tests passed. There was an error on report upload:

** (ExCoveralls.ReportUploadError) Failed to upload the report to 'https://coveralls.io' (reason: timeout).
    (excoveralls) lib/excoveralls/poster.ex:20: ExCoveralls.Poster.execute/2
    (mix) lib/mix/tasks/test.ex:385: Mix.Tasks.Test.run/1
    (mix) lib/mix/task.ex:316: Mix.Task.run_task/3
    (excoveralls) lib/mix/tasks.ex:51: Mix.Tasks.Coveralls.do_run/2
    (mix) lib/mix/task.ex:316: Mix.Task.run_task/3
    (mix) lib/mix/cli.ex:79: Mix.CLI.run_task/2
    (elixir) lib/code.ex:767: Code.require_file/2
The command "mix coveralls.travis" exited with 1.

Can you rerun the build?

josevalim · 2019-04-05T07:45:49Z

Can you rerun the build?

Done!

matlc · 2019-04-05T13:56:57Z

@josevalim Thanks José!

@eksperimental Great. I think this is ready for review. Its 4 commits, after squashing the main piece of work, then needing to fix a couple of things. Should be in better shape. Thanks!

josevalim · 2019-04-05T15:07:35Z

lib/ex_doc/formatter/html/search_items.ex

    encode(
      "#{id}.html##{HTML.text_to_id(header)}",
      "#{title} - #{header}",
+      "#{title} - #{header}",


The generated search_items.js is already really large, so I don't think we should add a new field. We already have a title field and we should re-use it. Therefore, can you please revert the changes to this file and re-use the existing file on the .js side of things? Thank you!

wojtekmach · 2019-04-05T16:03:29Z

I did a little bit of research (links below) and I wasn’t able to find any evidence that adding
the title attribute helps with SEO and in fact some sources discourage that as it unnecessarily
clutters the UI.

Some references claim that the only benefit of the title attribute is it improves
UX. If it boils down to that I’m not sure if it justifies the change.

The problem we were orignally trying to solve was to have Google “jump to” links, but it’s unclear
whether adding title helps here at all. Ruby doc that was mentioned as example of site that has
these jump to links, does not put title into the links.

We're also investigating a somewhat conflicting feature, which is adding popovers on hover: #971

For these reason, I think we shouldn’t move forward with this change. I’m sorry that I haven’t
raised these concerns sooner. Thank you for your work @CRYIC nonetheless.

To improve SEO perhaps we should go back to looking into
microformats mentioned in the earlier issues, or look into sitemaps (e.g. each package would have
it’s own sitemap)

This was by no means a comprehensive research, just a cursory look at some google hits when
searching for “seo link title”:

josevalim · 2019-04-05T16:27:27Z

Those are very good points @wojtekmach, thanks.

Looking at the entries above, they even recommend to not add titles if the content is the same as the title itself, as it pollutes the UI. With that said, I think the only case for adding links in today's docs would be in the typespecs (which was really well done by @CRYIC) but, again, as we are already adding popovers, it indeed feels the feature is redundant. :(

In any case, thanks for all the work @CRYIC, it is really appreciated ❤️ 💚 💙 💛 💜. Sorry that it won't be merged.

matlc · 2019-04-05T16:30:13Z

@wojtekmach Thank you for researching this a bit more. I had thought this might have been helpful originally, however, it does appear that probably isn't good. I had also thought that this would help for accessibility, but also that doesn't appear to the case either. Need to update my assumptions :) Thanks for your input. Looking forward to the popovers!

josevalim reviewed Dec 9, 2018

View reviewed changes

eksperimental suggested changes Dec 9, 2018

View reviewed changes

wojtekmach added this to the v0.20 milestone Dec 11, 2018

josevalim approved these changes Dec 17, 2018

View reviewed changes

milmazz approved these changes Dec 18, 2018

View reviewed changes

eksperimental suggested changes Dec 21, 2018

View reviewed changes

eksperimental suggested changes Feb 3, 2019

View reviewed changes

matlc force-pushed the master branch from 3835027 to 842236a Compare March 17, 2019 23:00

eksperimental requested review from josevalim and milmazz April 3, 2019 23:15

eksperimental self-requested a review April 3, 2019 23:23

michallepicki and others added 2 commits April 4, 2019 19:21

Merge branch 'master' of https://github.com/elixir-lang/ex_doc

db4b622

matlc force-pushed the master branch from 4def3ff to db4b622 Compare April 4, 2019 23:45

matlc added 2 commits April 4, 2019 19:57

Remove commented in git and fix duplicated search.js code

845c704

Update css/js assets after mix build

a331a27

josevalim reviewed Apr 5, 2019

View reviewed changes

wojtekmach closed this Apr 5, 2019

Add title attribute with mfa on links to improve seo #932

Add title attribute with mfa on links to improve seo #932

Uh oh!

Conversation

matlc commented Dec 9, 2018

Uh oh!

josevalim Dec 9, 2018

Choose a reason for hiding this comment

Uh oh!

matlc Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matlc commented Dec 9, 2018

Uh oh!

eksperimental commented Dec 9, 2018

Uh oh!

matlc commented Dec 16, 2018

Uh oh!

milmazz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milmazz Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

matlc Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

matlc commented Dec 18, 2018

Uh oh!

eksperimental commented Dec 21, 2018

Uh oh!

eksperimental left a comment

Choose a reason for hiding this comment

Uh oh!

eksperimental left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matlc commented Jan 2, 2019

Uh oh!

matlc commented Feb 3, 2019

Uh oh!

josevalim commented Feb 3, 2019 via email

Uh oh!

eksperimental commented Feb 3, 2019

Uh oh!

matlc commented Feb 3, 2019

Uh oh!

eksperimental left a comment

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Feb 12, 2019

Uh oh!

matlc commented Feb 12, 2019

Uh oh!

matlc commented Feb 12, 2019

Uh oh!

matlc commented Mar 17, 2019

Uh oh!

matlc commented Mar 31, 2019

Uh oh!

eksperimental commented Apr 3, 2019

Uh oh!

eksperimental commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eksperimental commented Apr 3, 2019

Uh oh!

matlc commented Apr 4, 2019

Uh oh!

matlc Dec 9, 2018 •

edited

Loading

eksperimental left a comment •

edited

Loading

milmazz left a comment •

edited

Loading

eksperimental left a comment •

edited

Loading

eksperimental left a comment •

edited

Loading

eksperimental commented Apr 3, 2019 •

edited

Loading

matlc commented Apr 5, 2019 •

edited

Loading

josevalim commented Apr 5, 2019 •

edited

Loading