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

s9y-db importer permalink, tag, excerpt, and semantic HTML improvements #392

Merged
merged 20 commits into from
Apr 24, 2021
Merged

s9y-db importer permalink, tag, excerpt, and semantic HTML improvements #392

merged 20 commits into from
Apr 24, 2021

Conversation

judebert
Copy link
Contributor

@judebert judebert commented Jan 3, 2019

These commits support plugins and properties required to migrate judebert.com from S9Y to Jekyll. The features include:

  • support permalinks from the entryproperties plugin
  • duplicate S9Y "extended body" functionality with Jekyll excerpts
  • import tags and categories
  • convert iframes and Serendipity image/captions to semantic HTML figures
  • use relative links instead of absolute
  • entryincludes plugin

Documentation has been updated to describe the new functionality.

DirtyF
DirtyF previously requested changes Jan 3, 2019
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

Make sure tests pass on your machine by running script/cibuild

lib/jekyll-import/importers/s9y_database.rb Outdated Show resolved Hide resolved
lib/jekyll-import/importers/s9y_database.rb Outdated Show resolved Hide resolved
lib/jekyll-import/importers/s9y_database.rb Outdated Show resolved Hide resolved
@judebert
Copy link
Contributor Author

judebert commented Jan 6, 2019

All PR comments addressed. script/cibuild passes on my computer.

@ashmaroli

This comment has been minimized.

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Requesting corrections in documentation..

docs/_importers/s9ydatabase.md Outdated Show resolved Hide resolved
docs/_importers/s9ydatabase.md Outdated Show resolved Hide resolved
docs/_importers/s9ydatabase.md Outdated Show resolved Hide resolved
docs/_importers/s9ydatabase.md Outdated Show resolved Hide resolved
docs/_importers/s9ydatabase.md Outdated Show resolved Hide resolved
docs/_importers/s9ydatabase.md Outdated Show resolved Hide resolved
@judebert
Copy link
Contributor Author

Friendly ping: I think I pushed another update addressing all issues, about 3 weeks ago. Is there anything else I need to do to merge?

@ashmaroli
Copy link
Member

addressing all issues, about 3 weeks ago.

Apologies, @judebert.
This had slipped out of my head. Will post a fresh review shortly.

@ashmaroli ashmaroli dismissed DirtyF’s stale review January 31, 2019 06:35

Concerns have been addressed

Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

Sorry, but some more changes needed.. 😈

c.option "drafts", "--drafts", "Whether to export drafts as well"
c.option "markdown", "--markdown", "convert into markdown format (default: false)"
c.option "permalinks", "--permalinks", "preserve S9Y permalinks (default: false)"
c.option "excerpt_separator", "--excerpt_separator", "Demarkation for excerpts (default: '<a id=\"extended\"></a>')"
Copy link
Member

Choose a reason for hiding this comment

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

  • I personally feel --excerpt_separator is a bit long for a CLI flag. Could it be something shorter? If not, it is fine.
  • You've listed <a id="extended"></a> as the default value for this flag. Should users provide a HTML string to override the default?
  • "--excerpt_separator" is bare here. If it can receive an argument, it should be indicated in UPPERCASE. If not, this flag is considered as a Boolean type that toggles between true or false
  • Typo: Demarcation is spelt (or spelled) with a c not k

c.option "includeentry", "--includeentry", "Replace macros from the includeentry plugin (default: false)"
c.option "imgfig", "--imgfig", "Replace nested img and youtube divs with HTML figure tags (default: true)"
c.option "linebreak", "--linebreak", "Line break processing: wp, nokogiri, ignore (default: wp)"
c.option "relative", "--relative", "Convert links with this prefix to relative (default:nil)"
Copy link
Member

@ashmaroli ashmaroli Jan 31, 2019

Choose a reason for hiding this comment

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

--linebreak takes an argument and hence needs a parameter alongside it: e.g.
--linebreak ARG and if --relative doesn't take an argument, the default should be false.

# :imgfig:: Replace S9Y image-comment divs with an HTML figure
# div and figcaption, if applicable. Works for img and
# iframe.
# Default: true.
#
Copy link
Member

Choose a reason for hiding this comment

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

This separation is not present around other comments. Please remove.


# Wrap media in link, if any
if big_link
big = big_link.attribute("href")
Copy link
Member

Choose a reason for hiding this comment

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

If big is not being used anywhere else, I think it could be avoided entirely:

media = "<a href='#{big_link.attribute("href")}'>#{media}</a>"

@parkr
Copy link
Member

parkr commented Apr 24, 2021

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 7b1261f into jekyll:master Apr 24, 2021
jekyllbot added a commit that referenced this pull request Apr 24, 2021
@judebert judebert deleted the s9y-db-judebert branch July 8, 2021 15:06
@jekyll jekyll locked and limited conversation to collaborators Jul 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants