feat(html pipe): Default pipeline should include <section/>s #379

ramboz · 2019-06-15T00:33:50Z

Include proper sections in the HTML pipeline instead of plain <hr> sectioning

BREAKING CHANGE:

Remove context.content.sections and move the sections directly under
context.content.mdast
Adjust json schemas accordingly
Move the section yaml meta info to <node>.meta
Move the section types to the mdast <node>.meta.types

Is dependent on adobe/helix-cli#1037

fix #279

ramboz · 2019-06-15T00:35:11Z

Trying to gather some early feedback to validate the approach before spending too much time on re-writing the tests

trieloff

Looks good so far.

trieloff · 2019-06-17T08:00:52Z

src/html/get-metadata.js

@@ -19,7 +19,7 @@ const {

 function yaml(section) {
  const yamls = selectAll('yaml', section);
-  section.meta = obj(flat(map(yamls, ({ payload }) => payload)));
+  section.meta = Object.assign({}, obj(flat(map(yamls, ({ payload }) => payload))));


Why the additional Object.assign?

👍 Some left-overs from testing various approaches. I'll definitely remove in the final PR.

trieloff · 2019-06-17T08:02:34Z

src/html/split-sections.js

@@ -11,8 +11,15 @@
 */

 const between = require('unist-util-find-all-between');
+const { selectAll } = require('unist-util-select');
+const { flat, obj, map } = require('@adobe/helix-shared').sequence;


I'd import @adobe/ferrum directly

trieloff · 2019-06-17T08:03:30Z

src/html/get-metadata.js

@@ -144,7 +144,8 @@ function fallback(section) {
 }

 function getmetadata({ content }, { logger }) {
-  const { sections } = content;
+  const { mdast: { children } } = content;
+  const sections = children.filter(node => node.type === 'root');


Can this work? Below, you are changing the type to section.

👍 Again, some left-overs from playing around… though it should have failed somehow. Will double-check

trieloff · 2019-06-17T08:03:51Z

src/html/split-sections.js

-      return {
-        type: 'root',
+      const section = {
+        type: 'section',


New node type is a good idea.

Is it ok to just inline the node type or do we have a central place to expose them as enum constants that I could use?

maybe a schema would be in order?

@rofe I added a schema, and updated existing ones accordingly with the changes.
Was just wondering if we have a sort of global "constants" file where I would be pulling the string from, or if it is ok to just inline for now

trieloff · 2019-06-17T08:06:27Z

src/schemas/mdast.schema.json

@@ -76,15 +77,21 @@
        "imageReference": "A pointer to an image",
        "footnote": "A footnote",
        "footnoteReference": "A reference to a footnote",
-        "embed": "Content embedded from another page, identified by the `url` attribute."
+        "embed": "Content embedded from another page, identified by the `url` attribute.",
+        "section": "The extracted sections of the document"


Suggested change

"section": "The extracted sections of the document"

"section": "A section within the document. Sections serve as a high-level structure of a single markdown document and can have their own section-specific front matter metadata."

trieloff · 2019-06-17T08:09:24Z

src/utils/section-handler.js

+ * @returns {string} The tag name for the section. Defaults to {@code div}.
+ */
+function getTageName(section) {
+  return (section.meta && section.meta.tagName) || 'div';


Can we not use camelCase for YAML properties? tagname should be sufficient.

trieloff · 2019-06-17T08:11:01Z

src/utils/section-handler.js

+ * @returns {string} The class name for the section. Defaults to {@code hlx-section}.
+ */
+function getClass(section) {
+  return (section.meta && section.meta.className) || 'hlx-section';


Same here, the YAML property should be class, even if it is slightly inconvenient to read it from JavaScript.

Also, what happens to the detected section types?

trieloff · 2019-06-17T08:12:47Z

src/utils/section-handler.js

+    // we have a section that is not the document root, so wrap it in the desired tag
+    if (parent && parent.type === 'root' && parent.children.length > 1) {
+      const tagName = getTageName(n);
+      const props = { class: getClass(n), 'data-hlx-types': getTypes(n) };


I'd rather merge the types into class and then have data-<prop for each additional prop that's in the section metadata.

@trieloff So, something like this?

<div class="hlx-Section hlx-Section--hasImage hlx-Section--nbImage1 hlx-Section--hasOnlyImage" data-hlx-<meta1>="<value1>" data-hlx-<meta2>="<value2>" …></div>

And we let the dev:

overwrite the section class

toggle whether type classes are added (and use the custom section class as prefix if set)

toggle whether additional meta props are exposed as data attributes

I went with the SUIT CSS naming convention, but we could also do:

<div class="hlx-section has-image nb-image-1 has-only-image" data-hlx-<meta1>="<value1>" data-hlx-<meta2>="<value2>" …></div>

We just run a higher risk of collisions with domain specific CSS class names

Yes, except that classnames should be lowercase and should be kept as they are (your lower example).

We can fix the collisions later, i.e. through a step that performs class renaming.

My reason for this is that the HTML is simpler, and writing CSS selectors for attributes is harder than for classes, so I'd err on the side of simplicity over consistency.

@trieloff If we assume a step that does the renaming, then does it make sense to let the author specify the section class name in the first place in the yaml? I could just hardcode hlx-section then.

Less logic => fewer risk of bugs

I think allowing to specify the class name is a useful quick fix for some scenarios.

i don't think we need to prefix the section with hlx-section as i think that the entire default DOM is a helix DOM and in case of a conflict the or some other desire to change the DOM it is very easily done in a the pre.js via jquery or regular DOM manipulation.

src/utils/section-handler.js

trieloff

Looks good so far. I guess eslint is still angry at you.

ramboz · 2019-06-18T15:05:00Z

@trieloff Mostly the tests are failing since I did not touch them yet. Wanted to first validate the approach

trieloff · 2019-06-18T16:43:08Z

Keep going, you are on the right track.

codecov · 2019-06-21T21:56:11Z

Codecov Report

Merging #379 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #379   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          48     50    +2     
  Lines        1100   1150   +50     
=====================================
+ Hits         1100   1150   +50

Impacted Files	Coverage Δ
src/utils/section-handler.js	`100% <100%> (ø)`
src/utils/conditional-sections.js	`100% <100%> (ø)`	⬆️
src/html/unwrap-sole-images.js	`100% <100%> (ø)`	⬆️
src/defaults/html.pipe.js	`100% <100%> (ø)`	⬆️
src/html/removeHlxProps.js	`100% <100%> (ø)`
src/html/split-sections.js	`100% <100%> (ø)`	⬆️
src/utils/mdast-to-vdom.js	`100% <100%> (ø)`	⬆️
src/html/get-metadata.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de63086...d985356. Read the comment docs.

ramboz · 2019-06-21T21:56:14Z

docs/content.schema.json

@@ -33,13 +33,6 @@
        "mdast": {
            "$ref": "https://ns.adobe.com/helix/pipeline/mdast"
        },
-        "sections": {


BREAKING CHANGE: Sections is removed and added to the mdast directly

ramboz · 2019-06-21T22:02:07Z

docs/mdast.schema.md

 | [label](#label) | `string` | Optional  | No | MDAST (this schema) |
 | [lang](#lang) | `string` | Optional  | Yes | MDAST (this schema) |
-| [meta](#meta) | `string` | Optional  | Yes | MDAST (this schema) |
+| [meta](#meta) | `object` | Optional  | Yes | MDAST (this schema) |


BREAKING CHANGE: the meta now contains the yaml meta information

ramboz · 2019-06-21T22:08:12Z

src/schemas/content.schema.json

@@ -33,13 +33,6 @@
    "mdast": {
      "$ref": "https://ns.adobe.com/helix/pipeline/mdast"
    },
-    "sections": {


BREAKING CHANGE: as above in the doc

ramboz · 2019-06-21T22:08:34Z

src/schemas/mdast.schema.json

@@ -148,9 +153,29 @@
      "format": "uri-reference",
      "description": "For resources, an url field must be present. It represents a URL to the referenced resource."
    },
+    "meta": {
+      "type": ["null", "object"],


BREAKING CHANGE: as above in the doc

ramboz · 2019-06-21T22:11:59Z

src/utils/section-handler.js

+
+const DEFAULT_SECTION_TAG = 'div';
+const DEFAULT_SECTION_CLASS = 'hlx-section';
+const SYSTEM_META_PROPERTIES = ['class', 'meta', 'tagname', 'types'];


YAML properties:

class {string}: will allow overwriting the default section class (i.e. hlx-section)

meta {boolean}: when set to true will output any additional YAML properties as data-hlx-* attributes

tagname {string}: will allow overwriting the default section tag (i.e. div)

types {boolean}: when set to true will output the section types as additional classes on the section (i.e. has-image, etc.)

Since meta and types are actual feature flags, should we follow a specific naming convention?
enable_meta/enable_types, output_meta/output_types, etc.

I would only do that once we expose it as a config parameter.

ramboz · 2019-06-21T22:24:06Z

test/fixtures/sections/2images.json

@@ -1,42 +1,40 @@
-[


Just unwrapping the array since we unwrap sole sections directly on the mdast

ramboz · 2019-06-21T22:28:16Z

test/fixtures/sections/complex.json

-        }
+{
+  "type": "root",
+  "children": [


Just wrapping the array inside the mdast root's children

ramboz · 2019-06-21T22:31:11Z

test/fixtures/sections/headerimage.json

@@ -1,50 +1,48 @@
-[


Just unwrapping the array since we unwrap sole sections directly on the mdast

(same for rest of the sections fixtures below)

ramboz · 2019-06-21T22:32:52Z

test/testConditionalSections.js

@@ -441,7 +453,7 @@ Or that one at the same time, because they are both part of an A/B test.
    // format, section metadata, the mdast format or the input above, you probably
    // want to change one of these values until you find one that *happens* to work
    // again...
-    assert.notDeepEqual(await runpipe('foo'), await runpipe('bang'));
+    assert.notDeepEqual(await runpipe('foo'), await runpipe('bar'));


As per existing comment above, the test started failing after my changes, so tried out some other value

ramboz · 2019-06-21T22:33:49Z

test/testGetMetadata.js

@@ -29,7 +29,7 @@ function callback(body) {
  parseFront(dat);
  split(dat, { logger });
  getmetadata(dat, { logger });
-  return dat.content.sections;
+  return dat.content.mdast;


Expose the mdast node directly since most tests have a single section that will be unwrapped to it

ramboz · 2019-06-21T22:35:15Z

test/testGetMetadata.js

-    assert.deepEqual(dat.content.meta, {});
-  });
-
-  it('getmetadata does not fail with missing sections', () => {


Removing this test since the empty mdast and missing sections are now the same.

Include proper sections in the HTML pipeline instead of plain <hr/> sectioning BREAKING CHANGE: Remove `context.content.sections` and move the sections directly under `context.content.mdast`; move the section types to the mdast `<node>.data.types`; move the section yaml meta info to `<node>.data.meta` fix #279

fix #279

Include proper sections in the HTML pipeline instead of plain <hr/> sectioning fix #279

refactor existing tests and adjust code to properly support the new sections in the mdast fix #279

fix #279

ramboz · 2019-06-23T16:50:01Z

src/html/get-metadata.js

@@ -158,9 +157,9 @@ function getmetadata({ content }, { logger }) {

  const img = sections.filter(section => section.image)[0];

-  content.meta = empty(sections) ? {} : sections[0].meta;


sections will never be empty as we fall back to the mdast node

trieloff

@ramboz merge when ready.

trieloff · 2019-06-24T15:08:21Z

@ramboz for an example of class look here: https://github.com/davidnuescheler/character-landing/blob/master/index.md

davidnuescheler · 2019-06-28T20:23:14Z

nice find @trieloff , of course that one didn't work yet.

i am really looking forward to having this implemented, looks great to me.

BREAKING CHANGE: section types are moved to the sectio's meta object fix #279

ramboz · 2019-06-28T20:59:01Z

After a quite lengthy discussion with @davidnuescheler, the conclusion is:

use a plain <div/> to wrap the section content. can be overridden with the tagname YAML property
use a default hlx-section class, but custom class can be added via YAML property
all valid HTML attributes in a YAML block are added as-is on the section element. Non-valid attributes are set as data-<attribute> instead.
```
---
title: foo
bar: baz
---
```
becomes
```
<div class="hlx-section" title="foo" data-bar="baz" …>…</div>
```
Section types (i.e. has-image is-image-only …) are moved back to the MDAST meta object (instead of directly on the meta). And exposed in the resulting DOM as data-hlx-types (edited)
Pipeline after step removes all hlx-* classes and data-hlx-* attributes in production code

ramboz · 2019-06-28T21:23:29Z

@trieloff As this requires changes in the IT tests in the cli, I'm not sure how to link the 2 branches together so the tests are passing. Any suggestion?

trieloff · 2019-06-29T15:17:26Z

@ramboz proceed carefully. Merge this branch, knowing that the smoke tests fail (and make sure they only fail in places you expect) then merge the fix in CLI.

ramboz · 2019-07-01T15:59:26Z

@trieloff Sounds good. Would have been great if the smoke test was smart enough to run from the same named branch instead of master in that case.

Anyway, haven't merged directly here before. Anything special in the procedure, or can I just go ahead and squash-merge, then update cli accordingly?

tripodsan · 2019-07-02T01:04:16Z

just go ahead and squash-merge, then update cli accordingly?

yes.

# [4.0.0](v3.7.4...v4.0.0) (2019-07-05) ### Features * **html pipe:** Default pipeline should include <section/>s ([#379](#379)) ([27ad875](27ad875)), closes [adobe/helix-cli#1037](adobe/helix-cli#1037) [#279](#279) ### BREAKING CHANGES * **html pipe:** - Remove `context.content.sections` and move the sections directly under `context.content.mdast` - Adjust json schemas accordingly - Move the section yaml meta info to `<node>.meta` - Move the section types to the mdast `<node>.meta.types`

adobe-bot · 2019-07-05T17:08:27Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ramboz requested review from koraa, trieloff, rofe and kptdobe June 15, 2019 00:34

trieloff reviewed Jun 17, 2019

View reviewed changes

rofe suggested changes Jun 17, 2019

View reviewed changes

src/utils/section-handler.js Outdated Show resolved Hide resolved

src/utils/section-handler.js Outdated Show resolved Hide resolved

trieloff reviewed Jun 18, 2019

View reviewed changes

rofe approved these changes Jun 18, 2019

View reviewed changes

ramboz commented Jun 21, 2019

View reviewed changes

ramboz changed the title ~~[WIP] feat(html pipe): Default pipeline should include <section/>s~~ feat(html pipe): Default pipeline should include <section/>s Jun 21, 2019

ramboz added 6 commits June 21, 2019 17:17

feat(html pipe): Default pipeline should include <section/>s

c133e36

fix #279

feat(html pipe): Default pipeline should include <section/>s

6144ee9

fix #279

feat(html pipe): Default pipeline should include <section/>s

15c06dd

Include proper sections in the HTML pipeline instead of plain <hr/> sectioning fix #279

refactor(html pipe): refactor code and tests for new sections support

5baff2d

refactor existing tests and adjust code to properly support the new sections in the mdast fix #279

docs(html pipe): Update section mdast documentation for sections

4fb87da

fix #279

refactor(html pipe): update meta schema

e72be05

fix #279

ramboz force-pushed the issues/279 branch from cd67384 to e72be05 Compare June 22, 2019 00:19

refactor(html pipe): remove unused dependency

a758b9c

fix #279

ramboz commented Jun 23, 2019

View reviewed changes

trieloff approved these changes Jun 24, 2019

View reviewed changes

feat(html pipe): Refactor according to updated requirements

e0832ef

BREAKING CHANGE: section types are moved to the sectio's meta object fix #279

Merge branch 'master' into issues/279

7a5f652

Merge branch 'master' into issues/279

d985356

ramboz merged commit 27ad875 into master Jul 5, 2019

ramboz deleted the issues/279 branch July 5, 2019 17:06

adobe-bot added the released label Jul 5, 2019

This was referenced Jul 12, 2019

Inject frontmatter properties of a section as DOM properties #355

Closed

Expose sections as documents (VDOM) #143

Closed

	"section": "The extracted sections of the document"
	"section": "A section within the document. Sections serve as a high-level structure of a single markdown document and can have their own section-specific front matter metadata."

		@@ -158,9 +157,9 @@ function getmetadata({ content }, { logger }) {

		const img = sections.filter(section => section.image)[0];

		content.meta = empty(sections) ? {} : sections[0].meta;

feat(html pipe): Default pipeline should include <section/>s #379

feat(html pipe): Default pipeline should include <section/>s #379

Conversation

ramboz commented Jun 15, 2019 • edited Loading

ramboz commented Jun 15, 2019

trieloff 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

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

ramboz Jun 17, 2019 • 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

trieloff left a comment

Choose a reason for hiding this comment

ramboz commented Jun 18, 2019

trieloff commented Jun 18, 2019

codecov bot commented Jun 21, 2019 • edited Loading

Codecov Report

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

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

trieloff left a comment

Choose a reason for hiding this comment

trieloff commented Jun 24, 2019

davidnuescheler commented Jun 28, 2019

ramboz commented Jun 28, 2019 • edited Loading

ramboz commented Jun 28, 2019

trieloff commented Jun 29, 2019

ramboz commented Jul 1, 2019

tripodsan commented Jul 2, 2019

adobe-bot commented Jul 5, 2019

ramboz commented Jun 15, 2019 •

edited

Loading

ramboz Jun 17, 2019 •

edited

Loading

codecov bot commented Jun 21, 2019 •

edited

Loading

ramboz commented Jun 28, 2019 •

edited

Loading