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.

Sign up for GitHub

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

Jump to bottom

html converter next low-hanging-fruit #4235

Closed
wants to merge 7 commits into from
Closed

html converter next low-hanging-fruit #4235

wants to merge 7 commits into from

Conversation

redbow-kimee
Copy link

No description provided.

@mojavelinux mojavelinux changed the title html converter next low-haning-fruit html converter next low-hanging-fruit Jan 5, 2022
@ggrossetie ggrossetie force-pushed the feature/html-converter-next branch 2 times, most recently from 34ca938 to d5f3f02 Compare March 31, 2022 08:41
@ggrossetie
Copy link
Member

Thanks @redbow-kimee!

It would be easier to review your contribution if each convert_ method was in a separated pull request. Would you be able to do that?

ggrossetie
ggrossetie reviewed Mar 31, 2022
View reviewed changes
lib/asciidoctor/converter/semantic_html5.rb Outdated
end

def convert_inline_button node
%(<samp class="button">#{node.text}</samp>)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think <samp> is the right element to represent a button. MDN says:

The <samp> element is used to enclose inline text which represents sample (or quoted) output from a computer program.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/samp

Having said that, I don't have a good alternative.
Currently, we are using <b>... I guess we could use <span>?

Copy link
Author

Choose a reason for hiding this comment

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

Yea, this one is tough, samp wasn't perfect, and I think b matches a bit less. span class="button" or something might be the "right" solution.

ggrossetie
ggrossetie reviewed Mar 31, 2022
View reviewed changes
lib/asciidoctor/converter/semantic_html5.rb
end

def convert_inline_break node
%(#{node.text}<br />)
Copy link
Member

Choose a reason for hiding this comment

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

An open question is whether we want to support XHTML or not. And if we want to support XHTML, should we always use / on void elements or use an option. Currently, we have an option named htmlsyntax to control if we should add / or not.

May I ask with the extra space? <br /> instead of <br/> ?

Copy link
Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong but XHTML5 is trivially HTML5, no? (maybe except some doctype nonsense). (I've been quite removed from web stuff for a while, so I'm probably rusty on this stuff).

As for <br /> vs. <br/>, I have no opinion, other than I've always put a personally.

Copy link
Member

Choose a reason for hiding this comment

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

It's mostly about adding a slash to close tags. For instance: <br> is valid HTML5 but invalid XHTML.

@redbow-kimee
Copy link
Author

It would be easier to review your contribution if each convert_ method was in a separated pull request. Would you be able to do that?

Sorry for the delay. What I'll do for now is isolate each of the convert_ers on a branch, except for the ones with open comments. (then mark off which ones have been isolated)

@redbow-kimee
Copy link
Author

@Mogztter I believe I've either successfully isolated each converter (in the case of images, pair of converters) to its own branch, Except for convert_[d|o|u]list, convert_admonition, convert_inline_break, and convert_inline_button which are all mentioned in threads on this MR.

@ggrossetie ggrossetie force-pushed the feature/html-converter-next branch from 704cccd to 23e117f Compare July 5, 2022 08:15
@ggrossetie ggrossetie force-pushed the kimee/html-converter-next branch 2 times, most recently from 2a61336 to 97a64ba Compare July 5, 2022 08:41
@ggrossetie
Copy link
Member

Thanks for your patience, I'm slowing merging your changes 🚋

@ggrossetie
Copy link
Member

We can now close this pull request, I've extracted the remaining convert_ methods into dedicated pull requests:

@ggrossetie ggrossetie closed this Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggrossetie ggrossetie ggrossetie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@redbow-kimee @ggrossetie