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

Add support for title suffix #1947

Merged
merged 3 commits into from
Jun 16, 2022
Merged

Conversation

EltonGohJH
Copy link
Contributor

@EltonGohJH EltonGohJH commented Jun 13, 2022

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Resolves #1946

Anything you'd like to highlight/discuss:
If there is no title but there is prefix and suffix, [prefix] - [suffix] will be displayed eg. perfixEg - suffixEg

Testing instructions:

Proposed commit message: Add support for title suffix


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Nice work @EltonGohJH 👍

Implementation looks good. Just a small suggestion for the documentation.

@@ -19,6 +19,7 @@ Here is a typical `site.json` file:
"baseUrl": "/myproduct",
"faviconPath": "myfavicon.png",
"titlePrefix": "FooBar Dev Docs",
"titleSuffix": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"titleSuffix": "",
"titleSuffix": "FooBar",

Just a suggestion: I think having an example string rather than an empty string will make it slightly better :)

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thanks for contributing to MarkBind @EltonGohJH :)

@jonahtanjz jonahtanjz added this to the 4.0 milestone Jun 13, 2022
Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Thanks @EltonGohJH for contributing!

A reminder on closing the related issue via the PR description, which is stated in the comments of the PR template in case you missed it (has already been edited)

If this pull request is addressing an issue, link to the issue: "Fixes #xxx" or "Resolves #xxx"
If the pull request completely addresses the issue, use one of the issue closing keywords. https://docs.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords

Also left a point below for discussion.

Otherwise LGTM 🚀

Comment on lines 128 to 133
const prefixedTitle = this.pageConfig.titlePrefix
? this.pageConfig.titlePrefix + (this.title ? TITLE_PREFIX_SEPARATOR + this.title : '')
: this.title;
const prefixedSuffixedTitle = this.pageConfig.titleSuffix
? (prefixedTitle ? prefixedTitle + TITLE_SUFFIX_SEPARATOR : '') + this.pageConfig.titleSuffix
: prefixedTitle;
Copy link
Contributor

@tlylt tlylt Jun 13, 2022

Choose a reason for hiding this comment

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

for discussion: the use of ternary statements here is fine but does make them slightly less readable. Personally I would have opted for a clearer style, perhaps via typical if-statements. What do you think?

Copy link
Contributor Author

@EltonGohJH EltonGohJH Jun 14, 2022

Choose a reason for hiding this comment

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

Idm changing it to use if else statement. I think using ternary statement is more consistent as the logic is more of less the same as prefixedTitle but with a longer variable name instead. (or should I change both)

Copy link
Contributor

Choose a reason for hiding this comment

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

@EltonGohJH Thanks for contributing to MarkBind! :)

I think if clauses to do the necessary modifications here might be simpler as well. But feel free to keep the current style if it turns out this is cleaner.

or should I change both

We try to keep (especially distinctively) unrelated changes elsewhere for collaboration, reviewing and tracing bugs / etc., but sometimes feature additions / enhancements can come with simplifications to existing implementations to make the whole easier to read. There isn't a clear cut guideline between doing these simplifications before or concurrently, which really depends on the change, but in this case the affected amount and complexity seems small enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ang-zeyu
The ternary expression seems more clean in my opinion. I think we can keep the ternary expression? The if loop seems way too verbose.

    let prefixedTitle;
    if (this.pageConfig.titlePrefix) {
      if (this.title) {
        prefixedTitle = this.pageConfig.titlePrefix + TITLE_PREFIX_SEPARATOR + this.title;
      } else {
        prefixedTitle = this.pageConfig.titlePrefix;
      }
    } else {
      prefixedTitle = this.title;
    }
    let prefixedSuffixedTitle;
    if (this.pageConfig.titleSuffix) {
      if (prefixedTitle) {
        prefixedSuffixedTitle = prefixedTitle + TITLE_SUFFIX_SEPARATOR + this.pageConfig.titleSuffix;
      } else {
        prefixedSuffixedTitle = this.pageConfig.titleSuffix;
      }
    } else {
      prefixedSuffixedTitle = prefixedTitle;
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

@EltonGohJH

was thinking something like this, keeping the best of both:

let title = this.title;
if (this.pageConfig.titlePrefix) {
  title = this.pageConfig.titlePrefix + (this.title ? TITLE_PREFIX_SEPARATOR + this.title : '')
}
if (this.pageConfig.titleSuffix) {
  ...
}

title = this.pageConfig.titlePrefix + (title ? TITLE_PREFIX_SEPARATOR + title : '');
}
if (this.pageConfig.titleSuffix) {
title = (title ? title + TITLE_SUFFIX_SEPARATOR : '') + this.pageConfig.titleSuffix;
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.

When drafting my previous comment, I didn't really consider the edge case of prefix + separator + empty title + separator + suffix that we need to handle to ensure prefix + one separator + suffix, which complicates this seemingly simple logic here 😓. I think this is clearer (probably in the future we can refactor this, should we decide to allow custom separators). In hindsight, it would be great if we could write a unit test for this :)

@ang-zeyu ang-zeyu merged commit 603c821 into MarkBind:master Jun 16, 2022
@tlylt
Copy link
Contributor

tlylt commented Jun 16, 2022

@all-contributors please add @EltonGohJH for code

@allcontributors
Copy link
Contributor

@tlylt

I've put up a pull request to add @EltonGohJH! 🎉

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.

Add option for configuring page title suffix
4 participants