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

Title will now be extracted from front matter if title: is present. #2510

Merged
merged 2 commits into from
Oct 23, 2018

Conversation

nagledb
Copy link
Contributor

@nagledb nagledb commented Oct 19, 2018

Builds on front matter changes introduced by #2347

@nagledb nagledb force-pushed the nagledb-front-matter-title branch from dc86600 to 7688631 Compare October 19, 2018 20:43
@nagledb
Copy link
Contributor Author

nagledb commented Oct 19, 2018

I updated the PR: Added preferences to support toggling front matter title extraction on/off and to customize what field is used for the title.

…n/off and to customize what field is used for the title.
@nagledb nagledb force-pushed the nagledb-front-matter-title branch from 7688631 to 3272033 Compare October 20, 2018 06:29
@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Oct 20, 2018
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, please change your code a bit before we can merge it 🎉

enableTableEditor: this.refs.enableTableEditor.checked
enableTableEditor: this.refs.enableTableEditor.checked,
enableFrontMatterTitle: this.refs.enableFrontMatterTitle.checked,
frontMatterTitleField: this.refs.frontMatterTitleField.value
Copy link
Member

Choose a reason for hiding this comment

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

I think you should trim() this string first. If I leave a leading blank space the title will be broken because <space>title != title and you should add test for that too 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think that change would definitely make sense but I'm not sure where to add it. It doesn't look like any of the other fields do any kind of validation/cleanup currently either. For example, I can change "Editor Font Size" to "12a" and I can change "LaTeX Inline Open Delimiter" to " $ " (two leading and two trailing spaces).

I also don't see that there's any existing test infrastructure for the UI tab or for the underlying ConfigManager.

Maybe that kind of stuff should all be done as a separate set of changes?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm...I guess you're right. I approve your change and maybe open an issue for it.

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Oct 20, 2018
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM

@Rokt33r Rokt33r added next release (v0.11.11) and removed approved 👍 Pull request has been approved by sufficient reviewers. labels Oct 23, 2018
@Rokt33r Rokt33r merged commit aeded9a into BoostIO:master Oct 23, 2018
@nagledb nagledb deleted the nagledb-front-matter-title branch October 23, 2018 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants