Skip to content
This repository has been archived by the owner on Feb 28, 2022. It is now read-only.

fix(sections): show sections in DOM #301

Closed
wants to merge 5 commits into from
Closed

Conversation

trieloff
Copy link
Contributor

@trieloff trieloff commented May 2, 2019

No description provided.

@davidnuescheler
Copy link
Contributor

as mentioned in #279 i think that it is counter intuitive to wrap an .md without any ThematicBreak into a <section>, so i am proposing a small change in the split-sections.js

@ghost
Copy link

ghost commented May 7, 2019

There were the following issues with this Pull Request

  • Commit: 9663eda
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@koraa
Copy link
Contributor

koraa commented May 8, 2019

@davidnuescheler Unfortunately I have to disagree about not using sections in a document without thematic breaks. Not wrapping those documents would create a big need for special casing and present an issue for the usability of our html output:

With wrapping you could always assume that: document.body.children is a list of sections in the htl if you want to get the content of the document; with this special case one would always test whether document.body.contains no section tags and then fall back to [document.body]. This seems a little esoteric I know, but I believe this would be a significant downside in many actual htl configuration (we may test this by looking for a few common htl usage patterns and seeing how the special casing affects them).

Another use case that may be a problem is styling with section { ... }; you could never assume sections are there.

Is there any actual issue with having a single section in a document? I believe unless some custom css or code uses sections, a section tag would pretty much not change the rendering in any way?

@davidnuescheler
Copy link
Contributor

I think I changed my mind and i am following @koraa @trieloff that it is better to special case the usecases where you need want to drop the wrapping section than to remove the sections by default if there is no ThematicBreak

also based on the discussion in #279 i think we should change this to <div class="section"> instead.

@ghost
Copy link

ghost commented May 8, 2019

There were the following issues with this Pull Request

  • Commit: 9663eda
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 86f111d
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

@ghost
Copy link

ghost commented May 8, 2019

There were the following issues with this Pull Request

  • Commit: 9663eda
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 86f111d
    • ✖ message may not be empty
    • ✖ type may not be empty
  • Commit: 4eecbc8
    • ✖ message may not be empty
    • ✖ type may not be empty

You may need to change the commit messages to comply with the repository contributing guidelines.


🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

*/
function section() {
return function handler(h, node, _, handlechild) {
const sectionnode = h(node, 'div', { class: 'section' });
Copy link
Contributor

@ramboz ramboz May 10, 2019

Choose a reason for hiding this comment

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

Maybe sectioNode?

Also, should we prefix the class to avoid potential collisions with other frameworks or the customer classes?

What about following something like https://suitcss.github.io/ and use hlx-Section or hlx-section?

@kptdobe
Copy link
Contributor

kptdobe commented Jun 11, 2019

@trieloff Is this still in progress ?

@trieloff trieloff closed this Jun 11, 2019
@kptdobe
Copy link
Contributor

kptdobe commented Jun 13, 2019

@trieloff I was more thinking of a merge than a close... ;) Any explanation why you closed it ? We need the change to clean up helix-pages.

@tripodsan tripodsan deleted the wip-split-sections branch July 18, 2019 01:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants