ENH: add icon_links option for custom navbar icons (with docs and pypi example) #293

bollwyvl · 2020-12-10T00:29:00Z

fixes Make social buttons extendable #270

This adds icon_links to html_theme_options, as well as labels to the existing few, and an example which points to PyPI (with a non-brand-style icon).

now (after Fix gitlab logo #296)
previously
- i liked the python one better, and snake looks cool, but is not free

As a screen reader might describe it:

adds a fixture test
- it raises the question whether the current built-in shortcuts should be before or after the custom ones
updates configuring docs
splits out icon links into a separate file
ensures custom icon styling for other fontawesome styles than brand

choldgraf · 2020-12-10T00:55:54Z

I think this is great, thanks @bollwyvl - my one question is whether we should just put the "dedicated social media" links through a deprecation cycle and recommend that all users use the generic "icon links" approach here. WDYT?

Also - could you add a test?

bollwyvl · 2020-12-10T03:52:41Z

Are there any good examples of good language for a deprecation like that? And are you thinking just in docs, or starting to throw warnings during builds? I hate being that guy 😢 but you're right, probably the right thing to do.

I'll get on the tests, though!

bollwyvl · 2020-12-10T04:01:50Z

And I'm not overly-attached to "icon_links" as a name either, but couldn't think of anything else at the right level of generality...

bollwyvl · 2020-12-10T04:39:22Z

Also would it be reasonable to add a id="navbar-icon-links" (or whatever we end up calling it)?

bollwyvl · 2020-12-10T05:12:22Z

Hm, as I'm looking at this: i wonder if it isn't worth taking a step back and thinking about some of the navbar stuff more broadly. Is it worth there being a (future) unification of external_links and icon_links?

choldgraf · 2020-12-11T12:52:19Z

hmmm - well the external links currently have a different place in the UI (next to the site's main links) rather than in the icons section. Plus, I think that ultimately we should be using Sphinx's functions to define the links in the navbar rather than our own custom functions (that way you'd specify external links in the navbar the same way you'd do so in any other sphinx site - via toctree. See #219 for a PR that did this but has since gotten a bit out-of-date...if people are willing to give it a relatively quick turnaround this time I'm happy to try to revitalize).

I think "icon links" is a reasonable name, esp if we keep it to icons-only :-) if we use this, we should define the icon links in a separate template so that downstream themes can reuse that section (e.g. via {{ include icon-links.html }}` etc

jorisvandenbossche · 2020-12-11T13:12:52Z

Thanks @bollwyvl!

Yeah, giving the different placement currently in the navbar, I am not sure combining it with external links is best. But at least this refactor to "icon_links" makes the pattern more consistent with "external_links".

I think "icon links" is a reasonable name, esp if we keep it to icons-only :-) if we use this, we should define the icon links in a separate template so that downstream themes can reuse that section (e.g. via {{ include icon-links.html }}` etc

+1

One advantage of keeping some of the most used "social links" instead of deprecating them, is that you don't need to know what the name of the fa icon is. Of course, good documentation could also alleviate that to some extent.

jorisvandenbossche · 2020-12-11T13:14:10Z

See #219 for a PR that did this but has since gotten a bit out-of-date...if people are willing to give it a relatively quick turnaround this time I'm happy to try to revitalize).

Sidenote here, I am planning to give this repo a bit more love over the holidays (plan ..), and that would certainly be on my radar then!

bollwyvl · 2020-12-11T13:21:12Z

Great, #219 sounds good if it allows for all of the flexibility needed. I'll concentrate on other stuff!

choldgraf · 2020-12-11T13:59:26Z

@bollwyvl to be clear #219 would only fix the "external links in the navbar" issue but not the original issue this PR addresses (icon links) so I think we should def get this one in as well!

@jorisvandenbossche good point about the short-hand, what if we allowed for another field in icon-links like kind: github/twitter/etc that would be a short-hand for fab fab-twitter and such. Then the person could either specify kind: twitter, url: twitter.com/choldgraf or they could choose the name/icon manually if they wished?

jorisvandenbossche · 2020-12-11T14:07:19Z

Indeed, we are going to want to keep the functionality of "icon_links" anyway, that's not directly related to #219

what if we allowed for another field in icon-links like kind: github/twitter/etc

Ah, yes, that could also be an option. Still another option to have this idea of shorthands, but simply for the "icon" field, instead of introducing another field? So for the "icon" field, we can check for some known values ("github", "twitter", etc), and otherwise assume it are correct fab names?

choldgraf · 2020-12-11T14:16:15Z

@jorisvandenbossche yeah I like that for "icon" - what if we just said "if icon is a single word, it's assumed to be one of the list of fab allowed names, and if it is 2 words, it's assumed to be <FA type> <FA name>"?

bollwyvl · 2020-12-11T14:45:07Z

ok, opened it back up! Though did make some progress on the pa11y-ci front, so will probably get that to PR-able before picking this back up.

so... the entire inventory of icons is available at build time:

wc node_modules/@fortawesome/fontawesome-free/metadata/icons.yml 
21485  37406 292522 node_modules/@fortawesome/fontawesome-free/metadata/icons.yml

an entry looks like this:

twitter-square:
  changes:
    - '1'
    - 5.0.0
  label: Twitter Square
  search:
    terms:
      - social network
      - tweet
  styles:
    - brands
  unicode: f081
  voted: false

We could dump that to JSON, stick it in the rendering context, and give us a lot of bang for the buck, e.g.

...
icon_links = [
  {"icon": "twitter-square"}
]

If icon is not a key in that file, just use it directly, no funny stuff.

If icon is a key in that file, i assume fa{icons[icon]["styles"][0][0]} fa-{icon} would do the trick.

It could also pull the label, etc. though Twitter Square looks funny, but it's better than no-label-for-you.

Heck, we could even generate a dumb python file that gave you autocomplete on the keys.

bollwyvl · 2020-12-11T21:55:56Z

Ok, mostly updated with review comments... I wasn't sure exactly what reuse/overload pattern is desired.

Also changed the label to name to be more in-line with external_links.

choldgraf · 2020-12-12T16:39:22Z

Interesting about the yml...the one thing I’m not sure of is where the prefix (eg fab vs fa etc) would be found, I don’t see that in the data file. Perhaps we can just assume fab is the default prefix, and then if people want to use the other fa prefixes they can specify manually?

bollwyvl · 2020-12-12T17:26:34Z

just assume fab

I don't think assuming anything about Other People's CSS is safe, over time. If someone wants a cat icon, give'm a cat icon, and don't expect it to be CatCorp. I do note that the CSS currently does special stuff for .fab and that selector should probably should be made more general, e.g. i[class*='fa-'] or more specific e.g. #navbar-icon-links i, and the example in the docs should pick from one of the other libraries (e.g. maybe the package or crate.

However: as I suggested in the code snippet, if we did hoist the YAML, reading off styles[0][0] will always yield one of [b]rand, [r]egular, or [s]olid. This would be easy to exhaustively test, as well, to avoid any future surprises when refreshing yarn.lock.

Anyhow: forgoing any data-driven stuff (which i will happily explore, on this PR or on a follow-up, if there's some agreement it would be useful), I'm not sad about sending people to the FontAwesome docs which are, well, awesome, and one can see all the info needed on the icon detail page to "do it the hard way":

choldgraf · 2020-12-12T17:30:25Z

Ahh I see now what you’re doing by pulling just the first letter of that field. Yeah that could definitely work. Though to your point about the FA docs being awesome, I feel like a good first step is to ask users to manually specify the icon according to the FA docs, and explore a future improvement of getting more clever with figuring out the right prefix and such.

bollwyvl · 2020-12-12T18:09:07Z

Actually, the bigger icons look like trash when used in other places. Will try some other stuff...

bollwyvl · 2020-12-12T18:33:30Z

One concrete thing we could do is have an icon test sheet in the docs site... though maybe that belongs to the "future work" piece...

jorisvandenbossche · 2020-12-23T18:03:18Z

I agree that for now, it seems perfectly fine to require people to specify the full fontawesome name (as it's basically copying that from the icon doc page on fontawesome docs). We can explore fancier automatic stuff later ;-)

Given that we currently include specific coloring for github/twitter, it's maybe fine to keep special casing this? (or at least keeping the css for it?)

jorisvandenbossche

Looks good!

jorisvandenbossche · 2020-12-23T18:04:23Z

docs/user_guide/configuring.rst

+
+      i.fa-twitter-square:before {
+         color: #55acee;
+      }


If we intend to keep that in our css, it might be worth mentioning that this is already done for twitter and github, so it's clear that you don't need to do the above manually for twitter/github?

added in d616236!

jorisvandenbossche · 2020-12-23T18:04:56Z

docs/user_guide/configuring.rst

   }

+
+The value of ``icon`` can be any full
+`FontAwesome 5 Free <https://fontawesome.com/icons?d=gallery&m=free>`__.


Seems to be missing a word in the sentence.

fixed in d616236!

jorisvandenbossche · 2020-12-23T18:08:11Z

src/scss/index.scss


+/* Social media buttons */
+
+i {


Should we include this in the navbar-icon-links selector above, so those colors are only used there?

limited in d616236!

choldgraf · 2021-03-01T01:01:03Z

hey @bollwyvl - this is a cool PR! Any interest in giving a little push to get it over the finish line?

bollwyvl · 2021-03-01T02:46:52Z

It appears all the comments were addressed... if there's anything else, please let me know!

jorisvandenbossche · 2021-03-07T15:10:22Z

src/scss/index.scss


+/* Social media buttons */
+
+.navbar-icon-links i {


Suggested change

.navbar-icon-links i {

#navbar-icon-links i {

jorisvandenbossche · 2021-03-07T15:11:12Z

Thanks for the updates!
One small thing: the colors didn't work (see https://pydata-sphinx-theme--293.org.readthedocs.build/en/293/), see my code suggestion, which I think should fix it

bollwyvl · 2021-03-07T17:23:07Z

@jorisvandenbossche thanks for the review! I moved the selector up, looking correct now on RTD:

choldgraf · 2021-03-07T17:31:06Z

This looks great to me! I think from a functionality perspective I am +1 on merging. I have one quick under-the-hood question. Do you think that the _templates folder is a better place for these links? https://github.com/pydata/pydata-sphinx-theme/tree/master/pydata_sphinx_theme/_templates

I think of _templates as being more like "components", little snippets of HTML that could be put in other parts of the page as well for themes that are sub-theming this one. The .html in folder above _templates I think of more as defining the major structure of the theme (sidebars, top bar, etc). What do folks think?

bollwyvl · 2021-03-07T20:51:02Z

I think raising the overall page architecture question to a higher-level issue is appropriate here... I'll take a stab at a strawman, but would echo not blocking this merge, rather we can refer to it as an example, perhaps.

The scope of what had to change (e.g. docs, gold master html) is not something I'd like to re-do/re-review at this stage in this PR... it is a relatively small change, and still touched 11 files!

choldgraf · 2021-03-07T20:58:27Z

pydata_sphinx_theme/docs-navbar.html

-        {% endif %}
-      </ul>
+      {%- block icon_links -%}
+      {%- include "icon-links.html" with context -%}


Suggested change

{%- include "icon-links.html" with context -%}

{%- include "_templates/icon-links.html" with context -%}

Just to be clear, I wasn't suggesting that we re-work the architecture of the layout in this PR, I just meant that we move icon-links.html to _templates/icon-links.html, and then make this change.

choldgraf · 2021-03-07T21:03:19Z

@bollwyvl good point that I don't think this should be blocking. If the suggestion seems like "no brainer" to you, then go for it, but if you think it's more nuanced than this, I suggest we just merge this in and then iterate in your discussion issue 👍

jorisvandenbossche · 2021-03-09T18:38:59Z

OK, let's keep it as a follow-up, and merge this. Thanks again @bollwyvl !

bollwyvl · 2021-03-09T18:44:08Z

Thanks all! Every little bit moves forward!

add icon_links with docs and pypi example

32276ef

bollwyvl mentioned this pull request Dec 10, 2020

Plan to improve accessibility documentation (and testing) #292

Closed

bollwyvl closed this Dec 11, 2020

bollwyvl reopened this Dec 11, 2020

bollwyvl mentioned this pull request Dec 11, 2020

Add pa11y testing and reporting #294

Merged

15 tasks

update with review comments

833abcd

only apply larger icon font in icon links nav

61e67c6

bollwyvl force-pushed the add-extensible-icon-links branch from 62ead20 to 61e67c6 Compare December 12, 2020 18:30

bollwyvl added 3 commits December 17, 2020 21:11

merge master, add gitlab purple

cf0e4e6

rebuild static

6e4e808

add gitlab_url to theme.conf

1ea56e4

bollwyvl mentioned this pull request Dec 18, 2020

Fix gitlab logo #296

Merged

jorisvandenbossche reviewed Dec 23, 2020

View reviewed changes

jorisvandenbossche mentioned this pull request Jan 22, 2021

How to use custom social media buttons with font-awesome icons? #306

Closed

bollwyvl added 4 commits February 28, 2021 21:29

merge upstream

6706ed3

rebuild

ad6c3fc

apply updates from review comments

d616236

rebuild

aa72fb2

jorisvandenbossche reviewed Mar 7, 2021

View reviewed changes

jorisvandenbossche changed the title ~~add icon_links with docs and pypi example~~ ENH: add icon_links option for custom icons (with docs and pypi example) Mar 7, 2021

jorisvandenbossche changed the title ~~ENH: add icon_links option for custom icons (with docs and pypi example)~~ ENH: add icon_links option for custom navbar icons (with docs and pypi example) Mar 7, 2021

move social media buttons into correct parent

356aa7c

choldgraf reviewed Mar 7, 2021

View reviewed changes

bollwyvl mentioned this pull request Mar 7, 2021

[discussion] Consider modularity of different page components #330

Closed

3 tasks

jorisvandenbossche merged commit e32af5f into pydata:master Mar 9, 2021

vkbo mentioned this pull request Apr 11, 2023

Invalid <label> tags in quick links menu #1287

Closed

	{%- include "icon-links.html" with context -%}
	{%- include "_templates/icon-links.html" with context -%}

ENH: add icon_links option for custom navbar icons (with docs and pypi example) #293

ENH: add icon_links option for custom navbar icons (with docs and pypi example) #293

Conversation

bollwyvl commented Dec 10, 2020 • edited Loading

choldgraf commented Dec 10, 2020

bollwyvl commented Dec 10, 2020

bollwyvl commented Dec 10, 2020

bollwyvl commented Dec 10, 2020

bollwyvl commented Dec 10, 2020

choldgraf commented Dec 11, 2020

jorisvandenbossche commented Dec 11, 2020

jorisvandenbossche commented Dec 11, 2020

bollwyvl commented Dec 11, 2020

choldgraf commented Dec 11, 2020 • edited Loading

jorisvandenbossche commented Dec 11, 2020

choldgraf commented Dec 11, 2020

bollwyvl commented Dec 11, 2020

bollwyvl commented Dec 11, 2020

choldgraf commented Dec 12, 2020

bollwyvl commented Dec 12, 2020

choldgraf commented Dec 12, 2020

bollwyvl commented Dec 12, 2020

bollwyvl commented Dec 12, 2020

jorisvandenbossche commented Dec 23, 2020

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jorisvandenbossche Dec 23, 2020

Choose a reason for hiding this comment

bollwyvl Mar 1, 2021

Choose a reason for hiding this comment

jorisvandenbossche Dec 23, 2020

Choose a reason for hiding this comment

bollwyvl Mar 1, 2021

Choose a reason for hiding this comment

jorisvandenbossche Dec 23, 2020

Choose a reason for hiding this comment

bollwyvl Mar 1, 2021

Choose a reason for hiding this comment

choldgraf commented Mar 1, 2021

bollwyvl commented Mar 1, 2021

jorisvandenbossche Mar 7, 2021

Choose a reason for hiding this comment

jorisvandenbossche commented Mar 7, 2021

bollwyvl commented Mar 7, 2021

choldgraf commented Mar 7, 2021

bollwyvl commented Mar 7, 2021

choldgraf Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

choldgraf commented Mar 7, 2021

jorisvandenbossche commented Mar 9, 2021

bollwyvl commented Mar 9, 2021 via email

bollwyvl commented Dec 10, 2020 •

edited

Loading

choldgraf commented Dec 11, 2020 •

edited

Loading

choldgraf Mar 7, 2021 •

edited

Loading