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

Fix li elements #5

Merged
merged 2 commits into from
Apr 7, 2024
Merged

Fix li elements #5

merged 2 commits into from
Apr 7, 2024

Conversation

gorango
Copy link
Contributor

@gorango gorango commented Apr 6, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Currently, li elements are not interpreted as blocks, so ul and ol elements get parsed into a single line of text.

toText(h('ul', [h('li', 'Foxtrot'), h('li', 'Golf')]))

Running the above yields 'FoxtrotGolf' instead of 'Foxtrot\nGolf'.

This PR enables the expected behaviour.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 6, 2024
@wooorm
Copy link
Member

wooorm commented Apr 6, 2024

Should they be? I guess I get it. But weird that I didn't add it before. Could you check the source of the blockOrCaption list, what's with li's there?

@gorango
Copy link
Contributor Author

gorango commented Apr 6, 2024

The blockOrCaption list contains menu, ol, and ul, but not any references to li in there nor in other parts of code. Similarly, dl is listed along with its descendants dd and dt.

Should they be?

I think so. Running .innerText on a ul with lis produces forced line breaks in Chrome and Firefox.

@wooorm
Copy link
Member

wooorm commented Apr 7, 2024

Hmm. Not sure. The URL above the list definitely shows that lis get a display: list-item. not block. And the place where blockOrCaption is used has the text from the spec that says “block-level or table caption”.

let node = document.createElement('ol');
let li1 = document.createElement('li');
let li2 = document.createElement('li');
li1.textContent = 'alpha';
li2.textContent = 'bravo';
node.appendChild(li1);
node.appendChild(li2);
console.log(node.innerText) // 'alphabravo'

edit: oh, if I append it to the DOM in between, it does add a \n

@wooorm
Copy link
Member

wooorm commented Apr 7, 2024

OK, I confirmed that display: list-item is for this algo the same as block.

let node = document.createElement('ol');
let li1 = document.createElement('l-i');
let li2 = document.createElement('l-i');
li1.style.display = 'list-item' // w/o this line, it’s `alphabravo`; w/o this line, w/ `li1` as `li`, it’s also `alpha\nbravo`
li1.textContent = 'alpha';
li2.textContent = 'bravo';
node.appendChild(li1);
node.appendChild(li2);
document.body.appendChild(node);
console.log(node.innerText)

lib/index.js Outdated Show resolved Hide resolved
Signed-off-by: Titus <tituswormer@gmail.com>
@wooorm wooorm changed the title Add support for li elements Fix li elements Apr 7, 2024
@wooorm wooorm merged commit 248fdc9 into syntax-tree:main Apr 7, 2024
2 checks passed

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Apr 7, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Apr 7, 2024
@wooorm
Copy link
Member

wooorm commented Apr 7, 2024

Thanks Goran! Released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

2 participants