Re: doc: add missing "id" attributes to extension packaging page - Mailing list pgsql-hackers

Hi Brar,

Here's my first review of the make_html_ids_discoverable.patch.


Overall:

To start with, I'd like to say I like the goal and how everything
works when the patch is applied.  I was a bit skeptical, thinking that
the whole thing was going to be distracting when reading the docs or
otherwise odd, but it really works.

The thought comes to mind to take accessibility into account.  I am
unclear on what that would mean in this case.


Regards CSS:

The CSS looks good.  Also, it passes the W3C CSS validator (
https://jigsaw.w3.org/css-validator/) for CSS level 3.


Regards XSLT:

I believe the XSLT needs work.

Without really knowing the style of PG's XSLT, I'm going to suggest
putting the comment
<!-- Make HTML ids discoverable (by hover) by adding links to the ids -->
above the new XSLT.  Precede the comment with 2 blank lines and follow
it by one blank line.  The idea is to make the code you're adding an
identifiable "separate section".

I have a question, it is the docbook html stylesheets for XML that are
being overridden?  It looks like it, and what else would it be, but....
(Sorry, I'm a bit stale on this stuff.)

I believe that overriding the XSLT by copying the original and making
modifications is the "wrong way" (TM).  I think that the right way is
to declare a xsl:default-mode somewhere in the stylesheets.  There
does not seem to be one, so the default mode for PG doc processing
could be something like "postgres-mode".  And I'd expect it to be
declared somewhere right at the top of the xml hierarchy.  (I forget
what that is, probably something like "document" or "book" or
something.)  Then, you'd write your for the <xsl:template
match="varlistentry"> and <xsl:template name="section.heading"> with a
mode of "postgres-mode", and have your templates call the "traditional
default", "modeless", templates.  That way your not copying and
patching upstream code, but are instead, in effect, calling it as a
subroutine.

This should work especially well since, I think, you're just adding
new output to what the upstream templates do.  You're not trying to
insert new output into the middle of the stock output or otherwise
modify the stock output.

You're adding only about 6 lines of XSLT to the upstream templates,
and copying 100+ lines.  There must be a better way.

See: https://www.w3.org/TR/xslt-30/#modes

I've never tried this, although I do recall doing something or another
with modes in the past.  And I've not gone so far as to figure out
(again?) how to call a "modeless template", so you can invoke the
original, upstream, templates.  And setting a default mode seems like
something of a "big hammer", so should probably be checked over by
someone who's more steeped in Postgres docs than myself.  (Like a
committer. :) But I believe it is the way to go.  To make it work
you'll need to figure out the XSLT mode selection process and make
sure that it first selects the "postgres-mode", and then the modeless
templates, and also still works right when a template calls another
and explicitly sets a mode.


Regards visual presentation:

Here's the fun question, what to have "appear" when a section or
varlistentry with an id is hovered over?

I kind of like your choice of the "#" character as the screen element
that becomes visible when you hover over a section or varlistentry
with the mouse, which then shows you the URL of the thing over which
you are hovering.  That's what's in the patch now.  But I wonder why
you chose it.  Is there some sort of standard?  I've seen the anchor
Unicode character before, (⚓) \u2693.  I don't find it particularly
helpful.  The "place of interest" symbol,(⌘) \u2318, might be nice.
There is (◎), \u25ce, the "bullseye" symbol.  There is the link symbol
(🔗), \U0001f517.  Like the anchor, it has generally been confusing
when I come across it.  The link symbol with the "text variant form",
(🔗︎) \U0001f517\ufe0e, looks more link an actual chain and is somewhat
better.  (The opposite of "text variant" is "emoji variant".)  There
is also the paperclip, (📎) \U0001f4ce.  And the paperclip text
variant, (📎︎) \U0001f4ce\ufe0e.

Of all the Unicode choices above, I think I prefer the last.  The text
paperclip.  📎︎

(The hex representations above are Python 3 string literals.  So
print("\U0001f4ce\ufe0e") prints the text paperclip.)

The actual representation of the various Unicode characters is going
to depend on the font.  (Things change for me when looked at in an
email v.s. in a browser.)

The question I have is should we use "Unicode icons" at all or is
plain old UTF-8 good enough for us (because it was good enough for our
ancestors ;) ?  Of course an image is also possible....  For now I'm
not going to advocate for a change from the "#" already used in the
patch.

Regards,

Karl <kop@karlpinc.com>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein