feat(parser/renderer/types): Support inline and image icons #613

gdamore · 2020-06-15T18:13:42Z

This adds inline icon support, and significantly refactors the icon handling
to bring in support for image based icons and all of the icon attributes
supported by asciidoctor, bringing us to feature parity.

Additionally, we preserve some of the attributes from the font icon tags onto
images, allowing flip, rotate, and size styles to be applied to icons.
(This is an improvement over asciidoctor, and requires supporting CSS.)

One subtle difference when comparing output. We emit source paths for images
without a leading "./". That is because our use of path.Join() automatically
removes the redundant portion.

Fixes #587
Fixes #611
Fixes #610

This adds inline icon support, and significantly refactors the icon handling to bring in support for image based icons and all of the icon attributes supported by asciidoctor, bringing us to feature parity. Additionally, we preserve some of the attributes from the font icon tags onto images, allowing flip, rotate, and size styles to be applied to icons. (This is an improvement over asciidoctor, and requires supporting CSS.) One subtle difference when comparing output. We emit source paths for images without a leading "./". That is because our use of path.Join() automatically removes the redundant portion. Fixes bytesparadise#587 Fixes bytesparadise#611 Fixes bytesparadise#610

Removed unused items, and consolidated down to just one icon type.

codecov · 2020-06-15T18:28:55Z

Codecov Report

Merging #613 into master will increase coverage by 0.09%.
The diff coverage is 92.18%.

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
+ Coverage   86.69%   86.78%   +0.09%     
==========================================
  Files          68       69       +1     
  Lines        4239     4322      +83     
==========================================
+ Hits         3675     3751      +76     
- Misses        360      363       +3     
- Partials      204      208       +4

xcoulon

This looks great! I just put some comments to ask you to add tests on other types of elements, to make sure that inline icons and images are properly parsed everywhere. But I believe that there's no need to verify all combinations of attributes again ;)

xcoulon · 2020-06-15T19:11:46Z

pkg/parser/parser.peg

@@ -388,7 +388,8 @@ TitleElement <- element:(Word
        / Space+ 
        / CrossReference
        / InlinePassthrough
-        / InlineImage 
+        / InlineIcon
+        / InlineImage


could you add a few tests just to make sure that inline icons/images can be parsed in title elements?

So it turns out that we only render plain here. (Just like for images.) It's possible that this decision could be revisited. I think the ability to render images and icons inside a title could be useful. What will happen now is you'll get the Alt text.

Ah, I guess that only happens for section 1.

Oh, wait. I guess its for the table of contents only. Lol.

xcoulon · 2020-06-15T19:12:05Z

pkg/parser/parser.peg

@@ -717,6 +718,7 @@ LabeledListItemTermElement <- element:(Word
        / ConcealedIndexTerm
        / IndexTerm
        / InlinePassthrough
+        / InlineIcon


could you add a few tests just to make sure that inline icons/images can be parsed in labeled list terms?

I'm doing this .. so they only work in labeled list descriptions.

The grammar does not permit any markup in the terms. This should probably be considered a defect, but not one that I'm going to fix right now.

(It's a preexisting defect.)

xcoulon · 2020-06-15T19:12:22Z

pkg/parser/parser.peg

@@ -964,6 +967,7 @@ DoubleQuoteBoldTextElement <- Word
        / MonospaceText
        / SubscriptText
        / SuperscriptText
+        / InlineIcon


could you add a few tests to verify that inline icons/images can be parsed in quoted text?

pkg/renderer/sgml/html5/icon.go

pkg/types/types.go

Co-authored-by: Xavier Coulon <[email protected]>

The definition lists and the ToC renderer didn't handle icons properly.

xcoulon · 2020-06-16T06:06:56Z

pkg/parser/icon_test.go

+			})
+
+			It("inline icon in quoted text", func() {
+				source := `an _italicized icon:warning[] message_`


sorry to insist, but could you add a test for each kind of quoted text? (bold, monospace, etc.)

Ugh. Ok. The parser tests are a royal pain. I have come to really despise the test framework -- it is almost impossible to read the output when there is an error sometimes.

you can try something like this:

ginkgo -focus "my test" pkg/parser -- -debug

with the -debug flag passed to the test, you'll have a dump of the actual result, so it's sometimes easier to compare with the expectation. Sometimes the diff is enough, but not always

xcoulon · 2020-06-16T06:44:51Z

This adds inline icon support, and significantly refactors the icon handling
to bring in support for image based icons and all of the icon attributes
supported by asciidoctor, bringing us to feature parity.

Additionally, we preserve some of the attributes from the font icon tags onto
images, allowing flip, rotate, and size styles to be applied to icons.
(This is an improvement over asciidoctor, and requires supporting CSS.)

One subtle difference when comparing output. We emit source paths for images
without a leading "./". That is because our use of path.Join() automatically
removes the redundant portion.

Fixes #587
Fixes #611
Fixes #610

next time let's try to have a single PR per issue, so it's easier to review. But I imagine that you found out the bugs while working on the PR and probably wanted to address them immediatley?

gdamore · 2020-06-16T06:46:25Z

Yes, it would have been a pain to do this in separate PRs, the fixes were not easily separable. I filed the tickets to log the issue, but splitting the work up not so easy.

xcoulon · 2020-06-16T06:48:20Z

Yes, it would have been a pain to do this in separate PRs, the fixes were not easily separable. I filed the tickets to log the issue, but splitting the work up not so easy.

sure, I understand the context here. It's just a good practice I want us to have here, to make PR focused and reviews simpler ;)

xcoulon · 2020-06-16T18:42:11Z

thanks @gdamore 🙌

gdamore added 2 commits June 15, 2020 11:11

lint fixes.

cff7bd7

Removed unused items, and consolidated down to just one icon type.

fix comment on AttrWindow

eed2a50

gdamore added this to the v0.5.0 milestone Jun 15, 2020

xcoulon reviewed Jun 15, 2020

View reviewed changes

gdamore and others added 3 commits June 15, 2020 12:25

Update pkg/types/types.go

932785a

Co-authored-by: Xavier Coulon <[email protected]>

Update pkg/types/types.go

c033248

Co-authored-by: Xavier Coulon <[email protected]>

Code review feedback, and fixes.

51f6d80

The definition lists and the ToC renderer didn't handle icons properly.

gdamore mentioned this pull request Jun 16, 2020

fix support for : (and inline images, icons) in definition list terms #615

Merged

xcoulon reviewed Jun 16, 2020

View reviewed changes

more quoted text tests

797f9a6

xcoulon approved these changes Jun 16, 2020

View reviewed changes

xcoulon merged commit fb7414e into bytesparadise:master Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parser/renderer/types): Support inline and image icons #613

feat(parser/renderer/types): Support inline and image icons #613

gdamore commented Jun 15, 2020

codecov bot commented Jun 15, 2020 •

edited

Loading

xcoulon left a comment

xcoulon Jun 15, 2020

gdamore Jun 16, 2020

gdamore Jun 16, 2020

gdamore Jun 16, 2020

gdamore Jun 16, 2020

xcoulon Jun 15, 2020

gdamore Jun 16, 2020

gdamore Jun 16, 2020

xcoulon Jun 15, 2020

gdamore Jun 16, 2020

xcoulon Jun 16, 2020

gdamore Jun 16, 2020

xcoulon Jun 16, 2020

xcoulon commented Jun 16, 2020

gdamore commented Jun 16, 2020

xcoulon commented Jun 16, 2020

xcoulon commented Jun 16, 2020

feat(parser/renderer/types): Support inline and image icons #613

feat(parser/renderer/types): Support inline and image icons #613

Conversation

gdamore commented Jun 15, 2020

codecov bot commented Jun 15, 2020 • edited Loading

Codecov Report

xcoulon 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

xcoulon commented Jun 16, 2020

gdamore commented Jun 16, 2020

xcoulon commented Jun 16, 2020

xcoulon commented Jun 16, 2020

codecov bot commented Jun 15, 2020 •

edited

Loading