Updating sidebars to use details/summary #789

tkadlec · 2018-08-29T18:42:15Z

Changes all necessary macro's to start using the <details> and <summary> elements instead of jQuery for the sidebar navigation.

Will close https://app.zenhub.com/workspace/o/mdn/sprints/issues/258

tkadlec · 2018-08-29T18:47:15Z

@schalkneethling @jwhitlock Would appreciate you folks having a look. This touched more than I had thought it would and I want to make sure I didn't overlook anything.

schalkneethling

Whoah, that is a lot of changes. Thanks @tkadlec. Some nits, questions, general comments, and required changes.

Note: Not all nits was introduced here but, seeing we are doing all this work, we might as well clean up a bit as well ;)

macros/APIRef.ejs

schalkneethling · 2018-09-04T09:13:56Z

macros/DocStatusQuickLinks.ejs

+                    <li><a href="/<%=env.locale%>/docs/MDN/Doc_status/JavaScript"><%=titleValues['JavaScript']%></a>
+                    <li><a href="/<%=env.locale%>/docs/MDN/Doc_status/Marketplace"><%=titleValues['Marketplace']%></a>
+                    <li><a href="/<%=env.locale%>/docs/MDN/Doc_status/MathML"><%=titleValues['MathML']%></a>
+                    <li><a href="/<%=env.locale%>/docs/MDN/Doc_status/SVG"><%=titleValues['SVG']%></a>


Missing closing tags for li tags

macros/DocStatusQuickLinks.ejs

macros/FirefoxOSSidebar.ejs

macros/HTMLMainQuickLinks.ejs

macros/nsprapiref.ejs

tests/macros/test-WebAssemblySidebar.js

schalkneethling · 2018-09-04T16:02:09Z

@tkadlec I notice that the details element has a bottom margin of 20px that is negatively affecting the layout of the sidebar. I am thinking we should remove that, but not for all details elements so, maybe something like

.toggle {
    ...
    details {
      margin-bottom: 0;
    }
}

schalkneethling · 2018-09-04T16:57:24Z

@tkadlec and that CSS change will be on the Kuma side ;)

a2sheppy · 2018-09-05T14:30:28Z

@jwhitlock tells me this gets rid of the links to the overview pages that are linked to from the expandable sections' headings. That's not ideal. I know they never worked right before, but it would be better if we could have that link functional instead:

> HTML
   <a>
   <abbr>
   ...

In that example above, "HTML" is an expandable heading; clicking it makes the elements below appear. What should happen is that there should be a widget you click to expand to show the element names, while allowing clicking the title "HTML" itself to take you to the main HTML reference page.

schalkneethling · 2018-09-05T15:10:49Z

Agreed @a2sheppy, very good point. @tkadlec ^^

jwhitlock

Thanks @tkadlec, this is a big one. I'd prefer if it was broken into multiple pull requests, but I understand a side requirement is to all go at once, to simplify the JS / CSS changes and be able to measure impact.

I think @schalkneethling identified most of the required changes, around missing EJS end tags %>. I think my required changes are:

HTMLSidebar.ejs: <%=text['HTML_Elements']%>
HTMLSidebar.ejs: Missing Global Attributes section
SVGRef.ejs: add missing closing tag </details>.

I just read the code this round, now to integrated testing. I'll see if a change is needed around data-default-state during that testing.

Update: Added SVGRef to required changes

macros/HTMLMainQuickLinks.ejs

macros/DocStatusQuickLinks.ejs

jwhitlock · 2018-09-05T13:58:05Z

macros/HTMLSidebar.ejs

-    </ol>
+  <li class="toggle">
+    <details <%=state('Introduction_to_HTML')%>>
+      <summary><%=text['Introduction_to_HTML']%></summary>


This drops the link, but it's also the first element in the details list, so 👍

There's a number of times that a section element has an <a> element in the existing sidebar, that is removed by this change. The translate attribute page has one of these sidebars, and if you hover over "HTML basics" -> "Introduction to HTML", you can see that the browser thinks clicking will go to Learn/HTML/Introduction_to_HTML. However, if you click the item, JavaScript takes over, and the section expands or collapses instead.

I'm not sure of a good way to combine "click here to expand / collapse" and "click here to go to this page". The arrow could be "collapse / expand" and the text could be "go to page", but that seems like a bad UI, making the common action the one with the tiny target.

This list has the pattern "move the summary link to the first item and add 'Overview'", and a few others do as well. This seems like a decent compromise going forward, and other sections should adopt it.

I'm trying to decide if this should be future work, since we're losing some information with this change. And I attached my essay to a line that doesn't need the change.

macros/HTMLSidebar.ejs

jwhitlock · 2018-09-05T15:19:37Z

macros/AddonSidebar.ejs

-    <li data-default-state="<%=currentPageIsUnder('WebExtensions/API')%>"><a href="<%=baseURL%>WebExtensions/API"><%=text['WebExtensions/API']%></a>
-     <%-template("WebExtAPISidebar", [])%>
+    <li class="toggle">
+      <details <%=currentPageIsUnder('WebExtensions/API')%>>


Nit: GitHub hates %>>. I guess EJS isn't popular enough for a robust syntax highlighter? 😉

The "Whitespace Slurping" ending tag may help appease GitHub and get the desired output:

<details <%=currentPageIsUnder('WebExtensions/API')_%> >

Nope, that seems worse. Maybe it doesn't like HTML escaping into an unquoted context?

<details <%- currentPageIsUnder('WebExtensions/API')_%> >

Nope, syntax highlighting just seems to hate us. No change needed.

All EJS (and Liquid/Jinja2/<insert templating language of choice here>) syntax highlighters I tried can’t properly highlight conditional attributes,

jwhitlock · 2018-09-05T15:26:52Z

macros/nsprapiref.ejs

+        <li><a href="/<%=locale%>/docs/Mozilla/Projects/NSPR/Reference/Introduction_to_NSPR#NSPR_Naming_Conventions"><%=text['NSPR Naming Conventions']%></a></li>
+        <li><a href="/<%=locale%>/docs/Mozilla/Projects/NSPR/Reference/Introduction_to_NSPR#NSPR_Threads"><%=text['NSPR Threads']%></a>
+          <ol>
+          <li><a href="/<%=locale%>/docs/Mozilla/Projects/NSPR/Reference/Introduction_to_NSPR#Thread_Schedoling"><%=text['Thread Scheduling']%></a>


Existing nit: #Thread_Scheduling, not #Thread_Schedoling. Although "Schedoling" should be a word...

tests/macros/test-WebAssemblySidebar.js

jwhitlock · 2018-09-05T15:48:23Z

tests/macros/test-toolssidebar.js

-  let links = dom.querySelectorAll('a');
-  assert.equal(links[1].textContent,  locales[locale].Page_Inspector);
+  let summaries = dom.querySelectorAll('summary');
+  assert.equal(summaries[0].textContent,  locales[locale].Core_Tools);


Weird test, but works after change. The HTML was:

<ol> <li><a href="#">Core Tools</a> <ol> <li><a href="/en-US/docs/Tools/Page_Inspector">Page Inspector</a></li>

and now is:

<ol> <li class="toggle"> <details> <summary>Core Tools</summary> <ol> <li><a href="/en-US/docs/Tools/Page_Inspector">Page Inspector</a></li>

I think the weirdness around testing sidebars is a symptom of a need for re-implementing them. Future work.

jwhitlock · 2018-09-07T13:33:07Z

@jwhitlock tells me this gets rid of the links to the overview pages that are linked to from the expandable sections' headings. That's not ideal. I know they never worked right before, but it would be better if we could have that link functional instead

@a2sheppy, if there is a way to make this work, then that would be great. It's not obvious to me that there is. I think most users want to expand / collapse the list, and a smaller number want to go to the other page. Making the title a link to the page makes the largest UI element the minority action, which I think is a bad user experience.

My mental model is application menus in macOS, where a menu item is either an item or expands a submenu, never mixed. If you have a UI example where actions are combined with submenus, that would help me wrap my head around your solution.

Otherwise, my inclination is to not expand the scope of this PR to "fix this UX problem". If we need to retain the links, my preference is to commit to the model where the first submenu item is the link to the overview menu. The side menu on the <img> page would then be:

<img>
> HTML Elements
  HTML Elements Overview
  A
     <a>
     <abbr>
     ...

I'm 50/50 on shipping @tkadlec's changes and adding overview items later, or making them part of this change.

schalkneethling · 2018-09-07T13:37:06Z

@jwhitlock I think the biggest thing is to not lose the links. I reckon your suggestion of adding another menu entry that links to the overview makes a lot of sense. It would also fix a bug that was opened a while back, complaining about the fact that you have to:

Right click the item
Copy the url
Paste it in the url bar

in order to get to the overview pages. +1

ExE-Boss · 2018-09-07T13:55:21Z

(In reply to @schalkneethling from #789 (comment))

@jwhitlock I think the biggest thing is to not lose the links. I reckon your suggestion of adding another menu entry that links to the overview makes a lot of sense. It would also fix a bug that was opened a while back, complaining about the fact that you have to:

Right click the item

Copy the url

Paste it in the url bar

in order to get to the overview pages. +1

You can also middle‑click to open the overview page in a new tab.

schalkneethling · 2018-09-07T13:59:09Z

@ExE-Boss True, you could also right click and open in a new tab or, command+click. Not an ideal user experience though, that is why I like @jwhitlock's idea.

…/kumascript into bug/258-moztogglr-reflow

tkadlec · 2018-09-09T03:19:22Z

@jwhitlock @schalkneethling @ExE-Boss I'd vote for John's solution, at least for a starting point.

Do I wrap that into this PR or do we break it out?

tkadlec · 2018-09-09T03:29:08Z

@tkadlec I notice that the details element has a bottom margin of 20px that is negatively affecting the layout of the sidebar

@schalkneethling Where are you seeing that? It should be taken care of here: mdn/kuma@11b4d07#diff-691645fb32733fcf070299b5bddeb224R56

jwhitlock · 2018-09-12T00:39:20Z

macros/ToolsSidebar.ejs

-        <li><a href="<%=baseURL%>Responsive_Design_Mode"><%=text["Responsive_Design_Mode"]%></a></li>
-        <li><a href="<%=baseURL%>Tips"><%=text["Tips"]%></a></li>
-      </ol>
+    <li class="toggle">


The {{ToolsSidebar}} was converted correctly 👍 as seen on https://developer.mozilla.org/en-US/docs/Tools/Web_Console . There's some local weirdness because the macro has https://developer.mozilla.org hard-coded into some links, which make them looks like external links in local dev. This should be removed, but it was in the old version, so out-of-scope for this PR.

jwhitlock · 2018-09-12T00:42:29Z

macros/WebAssemblySidebar.ejs

-      <li><a href="<%=baseURL%>/Caching_modules"><%=text['Caching_modules']%></a></li>
-      <li><a href="<%=baseURL%>/Exported_functions"><%=text['Exported_functions']%></a></li>
-    </ol>
+  <li class="toggle">


The {{WebAssemblySidebar}} macro was correctly updated, as seen on https://developer.mozilla.org/en-US/docs/WebAssembly/C_to_wasm

jwhitlock · 2018-09-12T00:54:07Z

macros/WebGLSidebar.ejs

-     <li><a href="/<%=locale%>/docs/Web/API/WebGL_API/Tutorial/Lighting_in_WebGL"><%=text['Lightning']%></a></li>
-     <li><a href="/<%=locale%>/docs/Web/API/WebGL_API/Tutorial/Animating_textures_in_WebGL"><%=text['Animating_textures']%></a></li>
-   </ol>
+  <li class="toggle">


The {{WebGLSidebar}} is converted correctly 👍, as seen on https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Tutorial/Getting_started_with_WebGL

jwhitlock · 2018-09-12T01:05:20Z

macros/WebRTCSidebar.ejs

-     <li><a href="/<%=locale%>/docs/Web/API/WebRTC_API/Session_lifetime"><%=text['Session_lifetime']%></a></li>
-     <li><a href="/<%=locale%>/docs/Web/API/WebRTC_API/Using_data_channels"><%=text['Using_data_channels']%></a></li>
-   </ol>
+  <li class="toggle">


The {{WebRTCSidebar}} macro is correctly converted 👍, as seen on https://developer.mozilla.org/en-US/docs/Web/API/WebRTC_API/Signaling_and_video_calling ("WebRTC Basics" in the sidebar):

jwhitlock · 2018-09-12T01:19:43Z

macros/eventref.ejs

@@ -21,11 +21,12 @@ output += '<li><strong><a href="' +  EventHref + '">Events</a></strong></li>';

 function buildEventList(name, events, open) {
  var openstr = open ? ' data-default-state="open"' : '';
-  var result = '<li'+ openstr +'><a href="#"><strong>' + name + '</strong></a><ol>';
+  <li class="toggle"><details open><summary>' + title + '</summary><ol></ol>
+  var result = '<li class="toggle"><details ' + openstr + '><summary>' + name + '</summary><ol>';


This isn't valid EJS, starting at <li class="toggle"> inside an EJS <% block. I think it should be something like:

var openstr = open ? ' open' : ''; var result = '<li class="toggle"><details ' + openstr + '><summary>' + name + '</summary><ol>';

It's valid now, but it should use the open attribute, not data-default-state="open". On https://developer.mozilla.org/en-US/docs/Web/Events/datachannel, the new sidebar (right side) is collapsed:

jwhitlock

There are still some missing closing EJS tags in the summary blocks.

jwhitlock · 2018-09-12T01:51:16Z

macros/nsprapiref.ejs

-      'Doc_Status' : 'NSPR Documentation status',
-      'The_MDN_project' : 'The MDN Project'
-
+    'NSPR' : 'NSPR',


{{nsprapiref}} is a big one, and it converted correctly 👍, as seen on https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/NSPR_Types:

jwhitlock · 2018-09-12T02:02:34Z

I've done a live test of each of the changed macros, and these need some fixes:

{{AddonSidebar}} - r216474286 - i18n text key fixes, "manifest.json" section to convert, indentation
{{HTMLRef}} - r216689743 - "HTML Elements" section to convert
{{HTMLSidebar}} - r216708844 - too much escaping for input
{{HTTPSidebar}} - Invalid EJS leads to rendering errors
{{SpiderMonkeySidebar}} - r216864956 - i18n key fix
{{eventref}} - Invalid EJS leads to rendering errors
{{jsctypesSidebar}} - Invalid EJS leads to rendering errors

schalkneethling · 2018-09-12T07:50:05Z

Thanks so much for all the detailed reviews @jwhitlock ☕️

tkadlec · 2018-09-27T17:53:46Z

@jwhitlock @schalkneethling I've gone through the sidebar's John flagged and tested each of them. Can you take another look when you have time?

schalkneethling

Couple of items to clean up still here, but we are soooo close. Thx @tkadlec

macros/APIRef.ejs

schalkneethling · 2018-09-28T13:28:56Z

macros/AddonSidebar.ejs

-        <li><a href="<%=baseURL%>WebExtensions/Examples"><%=text['WebExtensions/Examples']%></a></li>
-        <li><a href="<%=baseURL%>WebExtensions/What_next_"><%=text['WebExtensions/What_next_']%></a></li>
-      </ol>
+    <li class="toggle">


I am still seeing the above problems as mentioned above. I would also suggest that we add some padding inside the grey container.

macros/HTMLRef.ejs

tkadlec · 2018-09-28T14:36:28Z

@schalkneethling Thanks for the review! All of the issues are taken care of in the Kuma PR. The only thing that I need you to double-check is APIRef.ejs where I don't see the issues you're still seeing.

schalkneethling · 2018-09-28T14:38:19Z

The only thing that I need you to double-check is APIRef.ejs where I don't see the > issues you're still seeing.

All good, after pulling the latest and restarting all the things those issues were gone. I see there are two files with conflicts now though :/

tkadlec · 2018-09-28T14:41:23Z

Pushed just as your comment appeared for me. :)

schalkneethling

That's an r+ from me. Thanks @tkadlec 🌮

jwhitlock

These now work without errors, so I think we're ready to go. However, these changes will require re-rendering every page with the sidebars. Once the kuma style changes ship, the existing sidebars will get ugly until re-rendering. To apply the new sidebars, we'll need to re-render each page that uses them, which takes about 1 second per page (3-5 hours). I think we're going to aim for Thursday, October 4th for merging, deploying, and re-rendering.

Two small fixes we could still get in before Thursday:

open attribute in eventref.ejs, to auto-expand the details
Spacing in jsctypesSidebar

jwhitlock · 2018-09-28T17:21:00Z

macros/eventref.ejs

@@ -21,11 +21,12 @@ output += '<li><strong><a href="' +  EventHref + '">Events</a></strong></li>';

 function buildEventList(name, events, open) {
  var openstr = open ? ' data-default-state="open"' : '';
-  var result = '<li'+ openstr +'><a href="#"><strong>' + name + '</strong></a><ol>';
+  <li class="toggle"><details open><summary>' + title + '</summary><ol></ol>
+  var result = '<li class="toggle"><details ' + openstr + '><summary>' + name + '</summary><ol>';


It's valid now, but it should use the open attribute, not data-default-state="open". On https://developer.mozilla.org/en-US/docs/Web/Events/datachannel, the new sidebar (right side) is collapsed:

jwhitlock · 2018-09-28T17:28:38Z

macros/jsctypesSidebar.ejs

-
-  <li data-default-state="<%=state('Introduction')%>"><a href="/<%=locale%>/docs/Mozilla/js-ctypes/Using_js-ctypes"><%=text['Introduction']%></a>
-    <%-template("ListSubpagesForSidebar", ['/' + locale + '/docs/Mozilla/js-ctypes/Using_js-ctypes', 1])%>
+  <li class="toggle">


This compiles now. On https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes/js-ctypes_reference/CType, the new sidebar (right side) has more vertical spacing than before. I'm OK with the change, but @schalkneethling may have something to say about it.

.

Nope, all good. I like the new spacing.

jwhitlock · 2018-10-04T15:46:28Z

I'll address eventref.ejs in a new PR. The commits seems like a sequence of fixes, so squashing for merging, and adding bug 1488884.

Updating sidebars to use details/summary

eba7a81

tkadlec mentioned this pull request Aug 29, 2018

Front-end assets work for removing the MozToggler sidebar behavior mdn/kuma#4957

Merged

Updating macros tests

f676677

tkadlec mentioned this pull request Aug 29, 2018

Forced Reflow caused by MozToggler: Create side bar based on HTML5 mdn/sprints#258

Closed

4 tasks

schalkneethling suggested changes Sep 4, 2018

View reviewed changes

jwhitlock suggested changes Sep 5, 2018

View reviewed changes

This was referenced Sep 5, 2018

bug 1488884: Allow attribute <details open> mdn/kuma#4963

Merged

bug 1462475: Improve ./manage.py scrape_links mdn/kuma#4964

Merged

tkadlec added 9 commits September 8, 2018 21:34

adding missing closing tags for the list items

aa59a9a

Merge branch 'bug/258-moztogglr-reflow' of https://github.com/tkadlec…

4fee422

…/kumascript into bug/258-moztogglr-reflow

Removing empty line, indentation fix

07dd2ec

HTTPS links in the HTML Quick Links sidebar

40ee1bf

Cleaning up JS, HTTPS and MDN sidebars

742c06e

Clean up nsprapiref sidebar

621f68a

Tidying up tools and rtc sidebars

062fb9c

Adding global attributes section back

0cdcd0f

Elements to HTMLElements

e74eea3

tkadlec added 2 commits September 8, 2018 22:24

Removing unnecssary data-default-state

089f871

Closing details tag

3e9a346

jwhitlock reviewed Sep 12, 2018

View reviewed changes

jwhitlock suggested changes Sep 12, 2018

View reviewed changes

jwhitlock reviewed Sep 12, 2018

View reviewed changes

tkadlec added 9 commits September 26, 2018 21:27

AddOnSidebar fixes

f497335

Fixing Spider Monkey sidebar

b57999a

Fixing ejs errors in eventref

76ea6ca

Fixing ejs errors in jsctypes sidebar

4929ac5

HTML Elements toggleable

6e83b36

Fixing ejs errors on HTTPSidebar

01d9e2a

Fixing error in SpiderMonkeySidebar

3a5de95

Fixing formatting

70cb9f2

Escaping in HTML sidebar

4cd5ba9

schalkneethling suggested changes Sep 28, 2018

View reviewed changes

Merge branch 'master' into bug/258-moztogglr-reflow

7d1855e

schalkneethling approved these changes Sep 28, 2018

View reviewed changes

jwhitlock approved these changes Sep 28, 2018

View reviewed changes

jwhitlock merged commit 50a70d8 into mdn:master Oct 4, 2018

jwhitlock mentioned this pull request Oct 4, 2018

bug 1488884: Update eventref for <details> #924

Merged

ExE-Boss mentioned this pull request Oct 11, 2018

Modernize XSLT macros #734

Closed

2 tasks

Updating sidebars to use details/summary #789

Updating sidebars to use details/summary #789

Conversation

tkadlec commented Aug 29, 2018 • edited Loading

tkadlec commented Aug 29, 2018

schalkneethling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented Sep 4, 2018

schalkneethling commented Sep 4, 2018

a2sheppy commented Sep 5, 2018

schalkneethling commented Sep 5, 2018

jwhitlock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Sep 7, 2018

schalkneethling commented Sep 7, 2018

ExE-Boss commented Sep 7, 2018

schalkneethling commented Sep 7, 2018

tkadlec commented Sep 9, 2018

tkadlec commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Sep 12, 2018

schalkneethling commented Sep 12, 2018

tkadlec commented Sep 27, 2018

schalkneethling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkadlec commented Sep 28, 2018

schalkneethling commented Sep 28, 2018

tkadlec commented Sep 28, 2018

schalkneethling left a comment

Choose a reason for hiding this comment

jwhitlock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Oct 4, 2018

tkadlec commented Aug 29, 2018 •

edited

Loading

jwhitlock left a comment •

edited

Loading

jwhitlock left a comment •

edited

Loading

jwhitlock Sep 28, 2018 •

edited

Loading