prettify linked tests : Fixes #1374 #1487

shikhar-scs · 2018-02-08T04:14:16Z

As suggested by @marcoscaceres , the required changes mentioned at #1374 have been made. Here's a preview

Please preview

shikhar-scs · 2018-02-08T06:25:38Z

@marcoscaceres I have pushed in some new changes. Please review.

marcoscaceres

Some quick changes... I'm unsure about your usage of hyperHTML there also, but we can look at that after this round of fixes.

marcoscaceres · 2018-02-08T08:57:39Z

src/core/data-tests.js

    <a href="${href}">
-      ${href.split("/").pop()}
+      ${modifiedHref.split('-').join(' ')}


Upon reflection, I think I'd like us to keep the "-".

marcoscaceres · 2018-02-08T09:00:19Z

src/core/data-tests.js

@@ -23,11 +23,34 @@ export const name = "core/data-tests";
 const lang = defaultLang in l10n ? defaultLang : "en";

 function toListItem(href) {
-  return hyperHTML.bind(document.createElement("li"))`
+
+  let modifiedHref = href.split("/").pop().split(".html")[0];


nit:

const [modifiedHref] = href.split("/").pop().split(".html");

Also, might be worth checking if the tests always end with ".html"... Some might end with just ".htm", but double check in the web platform test repository.

marcoscaceres · 2018-02-08T09:01:21Z

src/core/data-tests.js

+
+  let emojiList = document.createElement('i');
+
+  let requiresConnectionEmoji = hyperHTML.bind(document.createElement("i"))` 🔒 `;


Any reason you chose i element in particular? I would figure maybe a span might be better here?

marcoscaceres · 2018-02-08T09:02:22Z

src/core/data-tests.js

+  let emojiList = document.createElement('i');
+
+  let requiresConnectionEmoji = hyperHTML.bind(document.createElement("i"))` 🔒 `;
+    requiresConnectionEmoji.setAttribute('aria-label','require-secure-connection');


Note that aria-label is read out by a screen reader, so we want to make this as friendly as possible. I would suggest instead: "requires a secure connection".

We have quite a few users with visual impairments, so we do our best to support their needs.

I really appreciate that 👏

marcoscaceres · 2018-02-08T09:04:07Z

src/core/data-tests.js

+  let requiresConnectionEmoji = hyperHTML.bind(document.createElement("i"))` 🔒 `;
+    requiresConnectionEmoji.setAttribute('aria-label','require-secure-connection');
+
+  let manualPerformEmoji = hyperHTML.bind(document.createElement("i"))` 💪 `;


As above, and aria label "the test must be run manually"

marcoscaceres · 2018-02-08T09:07:26Z

src/core/data-tests.js

+
+  let emojiList = document.createElement('i');
+
+  let requiresConnectionEmoji = hyperHTML.bind(document.createElement("i"))` 🔒 `;


These elements should only be created inside the if statements below. Otherwise, they are getting created unnecessarily if not needed (and then thrown away).

marcoscaceres · 2018-02-08T09:10:05Z

src/core/data-tests.js

+
+  let modifiedHref = href.split("/").pop().split(".html")[0];
+
+  let emojiList = document.createElement('i');


Nit: all the let should be const, as we are not reassigning them.

marcoscaceres · 2018-02-08T09:14:19Z

Just a general note that in the project we are using double quotes 😉

To pretty up the code, I've been using (prettier.js)[https://github.com/prettier/prettier] via Visual Studio Code... but also works in the command line.

marcoscaceres · 2018-02-08T09:16:15Z

@shikhar-scs, if you are unsure about any JS syntax stuff (e.g., when should I use const vs let vs var), please let me know. Happy to give you pointers to explain the difference.

shikhar-scs · 2018-02-08T09:17:03Z

Thank you for the review . I'll post the corrected changes soon👍

marcoscaceres · 2018-02-08T10:59:44Z

Thanks for continuing to hack in this. I’m just on my way home, so will take a look first thing tomorrow.

shikhar-scs · 2018-02-08T10:59:47Z

To pretty up the code, I've been using (prettier.js)[https://github.com/prettier/prettier] via Visual Studio

Thankyou for your suggestions and though I do l know the differences between var let and const, I'll ask you in case I have any doubts.

I've pushed some new changes. And as of now, please ignore the changes made to basic.html

I’m just on my way home, so will take a look first thing tomorrow.

No problem

marcoscaceres

The logic is mostly there, but a bit of refactoring needed here to keep this code easy to work with.

We can also do some optimizations, but trying to do the least amount of work possible.

marcoscaceres · 2018-02-09T04:06:23Z

examples/basic.html

@@ -3,8 +3,8 @@
  <head>
    <meta charset='utf-8'>
    <title>Replace me with a real title</title>
-    <script src='../js/deps/require.js' data-main='../js/profile-w3c-common' 
-      async class='remove'></script>
+    <script src='../js/deps/require.js' data-main='../js/profile-w3c-common'


Please revert changes to this file.

marcoscaceres · 2018-02-09T04:45:31Z

src/core/data-tests.js

+
+  // const modifiedHref = href.split("/").pop().split(".")[0];
+
+  if ( href.split("/").pop().split(".").includes("html") ||  href.split("/").pop().split(".").includes("htm") ) {


Nit: make this two steps, and combine the search (otherwise, you are doing things twice):

// the last item will be the filename const [testFile] = new URL(href).pathname.split("/").reverse(); // next we want to work out the type const testParts = testFile.split("."); const [testFile] = testParts; // the first item const isSecureTest = testParts.find(part => part === "https"); // this is probably incorrect, but find "manual" like this: const isManualTest = testFile.split("-").find(part => part === "manual"); // then if(isSecure){ // add secure emoji... } if(isManualTest) { // add manual emoji }

marcoscaceres · 2018-02-09T05:48:28Z

src/core/data-tests.js

@@ -24,34 +24,37 @@ const lang = defaultLang in l10n ? defaultLang : "en";

 function toListItem(href) {

-  // const modifiedHref = href.split("/").pop().split(".")[0];
+  var [testFile] = new URL(href).pathname.split("/").reverse();


nit: rename this to something else, don't reassign it below (otherwise it's confusing to know what this actually is)

marcoscaceres

Getting there 👍

marcoscaceres · 2018-02-09T05:49:57Z

src/core/data-tests.js

@@ -23,11 +23,42 @@ export const name = "core/data-tests";
 const lang = defaultLang in l10n ? defaultLang : "en";

 function toListItem(href) {
-  return hyperHTML.bind(document.createElement("li"))`
+
+  var [testFile] = new URL(href).pathname.split("/").reverse();


ok, call this something else and don't reassign it... otherwise, its confusing to know what this actually is.

marcoscaceres · 2018-02-09T05:50:40Z

src/core/data-tests.js

+    const requiresConnectionEmoji = hyperHTML.bind(document.createElement("span"))` 🔒 `;
+    requiresConnectionEmoji.setAttribute("aria-label","requires a secure connection");
+
+    console.log(testFile)


nit: remove console.log

marcoscaceres · 2018-02-09T05:51:09Z

src/core/data-tests.js

+    requiresConnectionEmoji.setAttribute("aria-label","requires a secure connection");
+
+    console.log(testFile)
+    testFile.split(".https").join("");


this line doesn't do anything :)

It does. It removes the .https parts from the end

shikhar-scs · 2018-02-09T06:20:54Z

Please review now @marcoscaceres . I've also squashed all my commits

marcoscaceres

Fixed up style and a few small bugs. Make sure you git pull the changes.

marcoscaceres · 2018-02-12T03:16:36Z

src/core/data-tests.js

+  const isSecureTest = testParts.find(part => part === "https");
+  const isManualTest = testFileName.split(".").join("-").split("-").find(part => part === "manual");
+
+  let emojiList = document.createElement("span");


nit: this should be a const, as it's not being reassigned, right?

marcoscaceres · 2018-02-12T03:28:33Z

src/core/data-tests.js

+
+  const isSecureTest = testParts.find(part => part === "https");
+  if (isSecureTest) {
+    const requiresConnectionEmoji = hyperHTML.bind(


It seems unnecessary to use hyperHTML here, and same in the isManualTest block below. Just create spans here, and don't bind them.

ok thanx for that 👍

marcoscaceres · 2018-02-12T03:28:48Z

src/core/data-tests.js

+    .split("-")
+    .find(part => part === "manual");
+  if (isManualTest) {
+    const manualPerformEmoji = hyperHTML.bind(


marcoscaceres · 2018-02-12T03:31:22Z

src/core/data-tests.js

    </a>
  `;
+  testList.append(emojiList);


Here, I think testList should "render" like this:

<a href="${href}"> ${testFileName} </a> ${emojiList}

marcoscaceres · 2018-02-12T03:33:55Z

src/core/data-tests.js

@@ -23,11 +23,50 @@ export const name = "core/data-tests";
 const lang = defaultLang in l10n ? defaultLang : "en";

 function toListItem(href) {
-  return hyperHTML.bind(document.createElement("li"))`
+  const emojiList = document.createElement("span");


Note, this should be an array really... otherwise, this could end up with either and empty span or with a nested set of spans:

<a href="...">name-of-test</a>  💪 

We want to end up with markup like this:

<li> <a href="...">name-of-test</a> 💪 🔒 </li>

This is still remaining to be fixed, right? (i.e., const emojiList = [];, instead of using a span)

marcoscaceres · 2018-02-12T03:38:27Z

src/core/data-tests.js

+  if (isManualTest) {
+    const manualPerformEmoji = hyperHTML.bind(
+      document.createElement("span")
+    )` 💪 `;


nit: we probably don't need the whitespace. We should add that between the spans. If we then want them to have more padding, we should add that using CSS.

ok I'll keep all of that in mind 👍

marcoscaceres · 2018-02-12T05:28:10Z

src/core/data-tests.js

-    )` 💪 `;
-    manualPerformEmoji.setAttribute(
+    const manualPerformEmoji = document.createElement("span")
+      manualPerformEmoji.innerHTML = `💪`;


Nit: note that the indentation will get reset by prettier.js

oh completely forgot about that. Apart from that any more changes you would like to see ?

Lastly, we would need to add tests to tests/spec/core/data-tests-spec.js, that check all the new changes we've made.

So, pretty close to being done here! Thanks for hanging in there!

shikhar-scs · 2018-02-14T08:19:11Z

@Marcos I've written the tests related to my code and pushed them here. Please review 😅

marcoscaceres · 2018-02-14T08:24:48Z

@shikhar-scs, just stepping out, but promise to review first thing tomorrow! Thanks for putting in the work.

shikhar-scs · 2018-02-14T08:29:59Z

@marcoscaceres Not an issue at all. I'll wait for tomorrow 👍

marcoscaceres

Just a couple of small things. Nearly there however!

marcoscaceres · 2018-02-15T03:16:10Z

src/core/data-tests.js

@@ -23,11 +23,50 @@ export const name = "core/data-tests";
 const lang = defaultLang in l10n ? defaultLang : "en";

 function toListItem(href) {
-  return hyperHTML.bind(document.createElement("li"))`
+  const emojiList = document.createElement("span");


This is still remaining to be fixed, right? (i.e., const emojiList = [];, instead of using a span)

marcoscaceres · 2018-02-15T03:28:07Z

tests/spec/core/data-tests-spec.js

@@ -17,6 +17,11 @@ describe("Core — data-tests attribute", () => {
            <a id="t1" data-cite="#test">a</a>
            <dfn id="t2" data-cite="#test">a</dfn>
          </p>
+          <p id="metadata-tests"
+            data-tests="this-is-secure.https.html,
+             this-is-manual.html, this-is-secure-and-manual.https.html">


Nit: put "this-is-secure-and-manual.https.html" on a new line, so it's visually distinct from the other two. Basically, we want:

data-tests=" this-is-secure.https.html, this-is-manual.html, this-is-secure-and-manual.https.html"

marcoscaceres · 2018-02-15T03:29:48Z

tests/spec/core/data-tests-spec.js

@@ -63,5 +68,33 @@ describe("Core — data-tests attribute", () => {
        expect(pathname.endsWith(`test${i}.html`)).toBe(true);
      });
    });
+    it(`adds emojis for manual and https related tests`, () => {
+
+      let test = doc.querySelector("#metadata-tests ul").querySelectorAll("li")


As you have the tests here, we should split the tests into three separate it() statements:

it("adds emojis for secure tests", () => { const li = doc.querySelector("#metadata-tests ul>li:nth-child(1)"); // other stuff }); it("adds emojis for manual tests", () => { const li = doc.querySelector("#metadata-tests ul>li:nth-child(2)"); // other stuff }); it("adds emojis for secure manual tests", () => { const li = doc.querySelector("#metadata-tests ul>li:nth-child(3)"); // other stuff });

marcoscaceres · 2018-02-15T03:33:44Z

tests/spec/core/data-tests-spec.js

+      let test = doc.querySelector("#metadata-tests ul").querySelectorAll("li")
+
+      // for test 1
+      expect(test[0].children[0].innerText.trim()).toEqual("this-is-secure");   // filename


might be better here:

const filename = li.innerText.trim(); expect(filename).toEqual("this-is-secure");

Then it's much more clear exactly what you are testing, same applies to the ones below. Basically, if you find that you have to leave a comment, e.g., // filename, that's a strong sign you should be using a variable.

note also here, the fact that you are calling .trim() seems like it might be a bug in the implementation. You shouldn't need to call trim, as the implementation should have trimmed it.

Fixed js style to match project.

Missing semicolons, etc.

marcoscaceres

🕺Awesome work! I ran the code through prettier, so no to bother you with nits.

Last little question and one small change, and we should be all good to go.

marcoscaceres · 2018-02-15T06:25:09Z

tests/spec/core/data-tests-spec.js

+    it(`adds emojis for secure tests`, () => {
+      const li = doc.querySelector("#metadata-tests ul>li:nth-child(1)");
+
+      const fileName = li.children[0].innerText.trim();


So, having to do "li.children[0]" and "li.children[1]" is not ideal here, because it's not clear what element you are working with. Can you please destructure the array into named variables, like (you can probably come up with better names):

const [fileElement, secureIconElement] = li.children;

Similarly for the ones below...

typo above, I mean "is not ideal"

Yeah Sure 👍

marcoscaceres · 2018-02-15T06:26:07Z

tests/spec/core/data-tests-spec.js

+      );
+      expect(spanAriaLabel2).toEqual("the test must be run manually");
+
+      const spanTitle2 = li.children[1].children[1].getAttribute("title");


Seems odd that they are nested? I think this might be a bug. The tree structure should be:

file-name emoji emoji

Or am I reading this wrong?

Its something like

<li><a>fileName</a>emojiemoji</li>

So, yeah roughly the same

shikhar-scs · 2018-02-15T07:15:32Z

@marcoscaceres I've refactored the code, removing all children[i] parts of code by destructuring the array. Also I realised that because of introducing the emojilist = [] instead of initialising it as a querySelector , the structure of the li element had to be changed a bit for selecting the spans .
Thanks for reminding.

Please review the code now.

marcoscaceres

Perfect! Looking forward to trying this out!

shikhar-scs · 2018-02-15T07:27:26Z

Ready for merge ??

marcoscaceres · 2018-02-15T07:29:11Z

yep, was just waiting for green light from TravisCI.

* develop: v19.6.0 feat: add icons to describe tests (#1487) nit(headers-spec): use %20 instead of space

marcoscaceres · 2018-02-15T07:32:24Z

respec@19.6.0 going live now. Great work @shikhar-scs!

marcoscaceres · 2018-02-15T07:37:33Z

Looking hella nice in a real spec (make sure you shift-refresh) https://w3c.github.io/payment-request/ 😻

shikhar-scs · 2018-02-15T07:44:43Z

Thanks a lot @marcoscaceres . My first contribution to respec 🎉🎉🎉

marcoscaceres requested changes Feb 8, 2018

View reviewed changes

shikhar-scs force-pushed the paint-up-linked-tests branch from 72a519e to 67a9747 Compare February 8, 2018 11:09

marcoscaceres requested changes Feb 9, 2018

View reviewed changes

marcoscaceres reviewed Feb 9, 2018

View reviewed changes

marcoscaceres requested changes Feb 9, 2018

View reviewed changes

prettify data-tests

24a229c

shikhar-scs force-pushed the paint-up-linked-tests branch from cea10e8 to 24a229c Compare February 9, 2018 06:10

Style cleanup

d758560

marcoscaceres requested changes Feb 12, 2018

View reviewed changes

shikhar-scs added 2 commits February 12, 2018 10:18

indentation changes

782a261

new changes

a5fbc19

marcoscaceres reviewed Feb 12, 2018

View reviewed changes

shikhar-scs added 2 commits February 12, 2018 11:50

yuyu

382e96f

test written for emojis

e60e02d

shikhar-scs mentioned this pull request Feb 14, 2018

Allow linking to algorithm steps #1425

Open

marcoscaceres requested changes Feb 15, 2018

View reviewed changes

shikhar-scs and others added 3 commits February 15, 2018 11:29

better documentation of tests

25a2c85

style: prettier

0d8a4a0

Fixed js style to match project.

fix nits

a3e4ed9

Missing semicolons, etc.

marcoscaceres requested changes Feb 15, 2018

View reviewed changes

shikhar-scs added 3 commits February 15, 2018 12:35

removed children[0] and destructered aaray

137837f

data-tests changes

f435879

conflicts in data-tests-spec

c2dd80b

Small bug fix, whitespace

d710b85

marcoscaceres approved these changes Feb 15, 2018

View reviewed changes

marcoscaceres merged commit ba657af into speced:develop Feb 15, 2018

marcoscaceres added a commit that referenced this pull request Feb 15, 2018

Merge branch 'develop' into gh-pages

7a60884

* develop: v19.6.0 feat: add icons to describe tests (#1487) nit(headers-spec): use %20 instead of space

shikhar-scs mentioned this pull request Mar 27, 2018

feat: punctuation checker for paragraphs #1589

Merged


		let emojiList = document.createElement('i');

		let requiresConnectionEmoji = hyperHTML.bind(document.createElement("i"))` 🔒 `;


		let modifiedHref = href.split("/").pop().split(".html")[0];

		let emojiList = document.createElement('i');


		// const modifiedHref = href.split("/").pop().split(".")[0];

		if ( href.split("/").pop().split(".").includes("html") \|\| href.split("/").pop().split(".").includes("htm") ) {

prettify linked tests : Fixes #1374 #1487

prettify linked tests : Fixes #1374 #1487

Conversation

shikhar-scs commented Feb 8, 2018

shikhar-scs commented Feb 8, 2018 • edited Loading

marcoscaceres left a 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

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

marcoscaceres commented Feb 8, 2018

marcoscaceres commented Feb 8, 2018

shikhar-scs commented Feb 8, 2018

marcoscaceres commented Feb 8, 2018

shikhar-scs commented Feb 8, 2018 • edited Loading

marcoscaceres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres left a 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

shikhar-scs commented Feb 9, 2018

marcoscaceres left a 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

Choose a reason for hiding this comment

marcoscaceres Feb 12, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shikhar-scs commented Feb 14, 2018

marcoscaceres commented Feb 14, 2018

shikhar-scs commented Feb 14, 2018

marcoscaceres left a 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

Choose a reason for hiding this comment

marcoscaceres left a comment

Choose a reason for hiding this comment

marcoscaceres Feb 15, 2018 • 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

shikhar-scs Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

shikhar-scs commented Feb 15, 2018

marcoscaceres left a comment

Choose a reason for hiding this comment

shikhar-scs commented Feb 15, 2018

marcoscaceres commented Feb 15, 2018

marcoscaceres commented Feb 15, 2018

marcoscaceres commented Feb 15, 2018

shikhar-scs commented Feb 15, 2018

shikhar-scs commented Feb 8, 2018 •

edited

Loading

shikhar-scs commented Feb 8, 2018 •

edited

Loading

marcoscaceres Feb 9, 2018 •

edited

Loading

marcoscaceres Feb 12, 2018 •

edited

Loading

marcoscaceres Feb 15, 2018 •

edited

Loading

shikhar-scs Feb 15, 2018 •

edited

Loading