Skip to content
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

Issue 752 example index #947

Closed
wants to merge 18 commits into from
Closed

Issue 752 example index #947

wants to merge 18 commits into from

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Nov 25, 2018

For issue #752, Updated the example index as a separate document using ReSpec.
Updated the generation of the example index to not use Glob, only uses fs (FileSystem) for node.


Preview | Diff

Copy link
Contributor

@nschonni nschonni left a comment

Choose a reason for hiding this comment

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

Looks like the ESLint config might need to be updated to look for a few more things

var fileNameTemplate = 'index.template';
var fileNameIndex = 'index.html';

ariaRoles = [
Copy link
Contributor

Choose a reason for hiding this comment

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

var = ariaRoles unless it's supposed to be a global variable

'tooltip'
];

ariaPropertiesAndStates = [
Copy link
Contributor

Choose a reason for hiding this comment

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

var = ariaPropertiesAndStates unless it's supposed to be a global variable

'aria-setsize'
];

indexOfRoles = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Global variable?


var sorted = [];

for (role in indexOfRoles) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorted = indexOfRoles.slice();


sorted = [];

for (prop in indexOfPropertiesAndStates) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorted = indexOfPropertiesAndStates.slice();

];

indexOfRoles = [];
indexOfPropertiesAndStates = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Global variable?

}

function getTitle (data) {
title = data.substring(data.indexOf('<title>') + 7, data.indexOf('</title>'));
Copy link
Contributor

Choose a reason for hiding this comment

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

var title = ...


sorted.sort();

html = '';
Copy link

@jkva jkva Nov 25, 2018

Choose a reason for hiding this comment

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

In places where HTML is generated via string concatenation, consider using a JS template literal with some utility functions, such as

const html = sorted.reduce((set, role) => {
  const examples = indexOfRoles[role];

  let examplesHTML = '';
  if (examples.length === 1) {
    examplesHTML = `<a href="${examples[0].ref}">${examples[0].title}</a>`;
  else {
    const exampleListItem = item => `<li><a href="${item.ref}">${item.title}</a></li>`;
    examplesHTML = `<ul>${examples.map(exampleListItem).join('\n')}</ul>`;
  }

  return set + `
    <tr>
      <td><code>${role}</code></td>
      <td>${examplesHtml}</td>
    </tr>
  `;
}, '');

It's available in Node 6 and up. Keep in mind you can still use var in place of let and const if you so wish.

@mcking65 mcking65 changed the base branch from master to issue752-add-index-utility December 4, 2018 01:51
@mcking65
Copy link
Contributor

mcking65 commented Dec 4, 2018

@jongund, what is the benefit of making the index into a separate respec doc? It gives the impression that the index is a separate publication.

I don't know if respec still supports doing so, but ARIA 1.0 had a version where each section was split into a separate page. For example, here is the intro:
https://www.w3.org/TR/wai-aria-1.0/introduction

So, respec might somehow be told to strip out all the redundant information and tie it back to the main doc. However, I don't think that buys us anything. It'd be much simpler to have a template that includes the header and footer info we want.

@mcking65
Copy link
Contributor

mcking65 commented Dec 4, 2018

Wow @jongund, this index is super useful!!

One side effect of respec is that it changes the heading content, which then makes for some very confusing table captions:

  1. Examples by Role §
  2. Examples By Properties and States §

I was particularly confused by the word "section", which is the accessible name Chrome calculates for the permalink. It gets included in the table caption along with the section number. Somehow it caused me to overlook the presence of the table ... then I looked at the code and understood.

@mcking65
Copy link
Contributor

mcking65 commented Dec 4, 2018

I see you are trimming the word "example". If the title starts with "example of ", like with the tabs examples, we should trim the " of " as well.

@mcking65
Copy link
Contributor

mcking65 commented Dec 4, 2018

@jongund, We probably want the example-index.js and index.template files in the root /scripts folder along with travis-deploy.sh, or perhaps in the root folder itself. I think it would be good to name the output file attribute-index.html instead of index.html. We have a plan to change the name of aria-practices.html to index.html.

@michael-n-cooper may have an opinion on this.

@michael-n-cooper
Copy link
Member

I'm afraid I can't tell what you're doing enough to have an opinion, I'm not a code whiz and there are a lot of commits in this pull request to try to figure out. I would have an opinion about the locations of files named index.html, the naming of files meant to serve as indices, etc. But I can't offer an opinion on this specific issue at this time.

@jongund
Copy link
Contributor Author

jongund commented Dec 4, 2018

Matt,
When we talked a few weeks ago about the index of examples, you felt the index should be a separate document. So I changed the code to create a separate document in the examples directory and assumed it would need to use the respec pattern. If there is a way to have it as part of the practices document I think that would be better and what my original code did. So If I understand correctly you want the script to create a code fragment with the index information that can be then be merged into the practices document using some respec technique?

@mcking65
Copy link
Contributor

mcking65 commented Dec 5, 2018

Jon, We do not have to use respec for it to a file in the examples directory. Just like none of the example pages are respec docs, the attribute-index.html file does not have to be a respec doc.

@mcking65
Copy link
Contributor

mcking65 commented Dec 5, 2018

I will ask Michael tomorrow if having the index in a separate file is better from a build and publication process perspective.

@jongund
Copy link
Contributor Author

jongund commented Dec 5, 2018

I am going to move this code over to the issue752-add-index-utility branch in the w3c/practices repo and just make a simple document for the index and look at the js coding recommendations in this issue to update the script.

@mcking65
Copy link
Contributor

mcking65 commented Dec 6, 2018

@jongund, that sounds great.

I did discuss this with Michael today. Here are his preferences:

  1. Leave the name of the file that contains the index as index.html. Michael prefers that we have an index.html in each of the high-level directories. So, ignore what I said about changing its name.
  2. Do not make the examples/index.html file into a respec doc. We will format it so it looks like it belongs to the rest of the document with an appropriate header and footer, and that is best done without using respec.
  3. Put the js in the root /scripts folder. You can also put index.template there, even though it is not a script; we really do not have a better place for it now.

After you have made these changes and considered the other recommendations above, we'll look for advice from @nschonni wrt integrating into the travis build.

Thank you again for taking this one on!!!

@jongund
Copy link
Contributor Author

jongund commented Dec 6, 2018

This pull request can be ignored, please see pull request 951 for the latest version of the index based on feedback from Matt King and Michael Cooper.

@mcking65
Copy link
Contributor

mcking65 commented Dec 7, 2018

Closing because replaced by pull request #951.

@mcking65 mcking65 closed this Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants