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

Allow pages to be excluded from search #296

Merged

Conversation

nusjzx
Copy link
Contributor

@nusjzx nusjzx commented Jun 29, 2018

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [X] Enhancement to an existing feature
• [ ] Other, please explain:

Resolves #269

What is the rationale for this request?
In some websites, there is heavy duplication of content. e.g., normal text book and a printable version of a text book. should allow some pages to be excluded from search results

What changes did you make? (Give an overview)
allow "searchable" : "no" in page or glob entries. when rendering site data, filter out these pages

Testing instructions:

  1. 2103 website: add "searchable" : "no" in {glob: "print.md"} entry
    before:
    image
    after:
    image
    no more printable entries.
    website: https://nusjzx.github.io/website-base/

@yamgent
Copy link
Member

yamgent commented Jun 29, 2018

  1. Update PR title: 269-Allow pages excluded from search -> Allow pages to be excluded from search (remove the issue number + add 'to be')

  2. Need to select the correct purpose of the PR:

• [ ] Documentation update
• [ ] Bug fix
• [ ] New feature
• [ ] Enhancement to an existing feature
• [ ] Other, please explain:

  1. I can't get the searchable attribute to work on the page's frontmatter. See this folder:

~`https://github.com/yamgent/markbind-problems/tree/master/pr_296_version_1~~

I added searchable: no to nextPage.md. Tried it on the live preview, I can still find nextPage.md:

Not relevant as per Prof's comment in #269.

@nusjzx nusjzx changed the title 269-Allow pages excluded from search Allow pages to be excluded from search Jun 29, 2018
Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Also update the documentation website to explain how to use the new searchable syntax.

lib/Page.js Outdated
@@ -60,6 +60,7 @@ function Page(pageConfig) {
this.frontMatter = {};
this.headings = {};
this.includedFiles = {};
this.searchable = pageConfig.searchable;
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this line below line 46, as the variables that are using pageConfig are all initialized there.

lib/Site.js Outdated
@@ -259,6 +259,7 @@ Site.prototype.createPage = function (config) {
sourcePath,
tempPath,
resultPath,
searchable: config.searchable,
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to below line 254 to maintain alphabetical order (only the sourcePath, tempPath, resultPath and asset are placed at the bottom).

lib/Site.js Outdated
// Add pages collected by walkSync without duplication
this.addressablePages = _.unionWith(this.addressablePages, globPaths,
this.addressablePages = _.unionWith(globPaths, this.addressablePages,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we swapped the order here?

Copy link
Member

Choose a reason for hiding this comment

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

As you explained face-to-face, the scenario in concern was this:

    {
      "src": "index.md",
      "title": "Hello World"
    },
    {
      "glob": "index.md",
      "searchable": "no"
    },

This will not work if the order is not swapped.

Copy link
Contributor

@acjh acjh Jul 1, 2018

Choose a reason for hiding this comment

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

Hmm, src should have higher priority than glob as it's more specific.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, src should have higher priority than glob as it's more specific.

@acjh agree with you on this. @damithc do you agree with this as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with src having higher priority than glob.

{
      "src": "index.md",
      "title": "Hello World"
    },
    {
      "glob": "index.md",
      "searchable": "no"
    },

In the case above, it looks like the "searchable": "no" should be applied because src (although of higher priority) doesn't specify the searchable property. i.e., specified should take priority over not specified. But I'm ok this is too hard to implement.

lib/Site.js Outdated
@@ -491,6 +494,7 @@ Site.prototype.generatePages = function () {
faviconUrl,
pageSrc: page.src,
title: page.title,
searchable: page.searchable ? (page.searchable !== 'no') : true,
Copy link
Member

@yamgent yamgent Jun 29, 2018

Choose a reason for hiding this comment

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

Can be simplified to just:

searchable: (page.searchable !== 'no'),

EDIT: Edited the syntax

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Just a grammar correction, the rest LGTM.

@@ -45,7 +46,7 @@ Let's examine a typical `site.json` file:
| **baseUrl** | The base url relative to your domain. Default: <code></code>&nbsp;(empty). For example, if you are using Github Pages to host your deployed website and it is published at `https://markbind.github.io/site-demo-cs2103`, then your `baseUrl` would be `/site-demo-cs2103`. You can use this variable for [specifying path reference](contentAuthoring.html#specifying-path-reference) of images and links. **You may need to change the `baseUrl` when deploying to a different repo.** |
| **faviconPath** | The location of the favicon. Default: `favicon.ico`. If the favicon was recently changed, you may need to force-refresh to see the new image. |
| **titlePrefix** | The prefix for all page titles. The separator <code>-</code> will be inserted by MarkBind. |
| **pages** | An array of pages to be rendered. The `src` is the file; `title` is the page title for the generated web page. Titles specified here take priority over titles specified in the [front matter](contentAuthoring.html#front-matter) of individual pages. Alternatively, `glob` can be used to define a file pattern. If `title` is not specified in the front matter, the page will have `titlePrefix` as its title. |
| **pages** | An array of pages to be rendered. The `src` is the file; `title` is the page title for the generated web page. Titles specified here take priority over titles specified in the [front matter](contentAuthoring.html#front-matter) of individual pages. Alternatively, `glob` can be used to define a file pattern. If `title` is not specified in the front matter, the page will have `titlePrefix` as its title. `"searchable": "no"` can be used to specified pages excluded from searching. If you need to exclude pages covered in both page entry and glob entry, you need to specify `"searchable": "no"` in page entry.|
Copy link
Member

Choose a reason for hiding this comment

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

"searchable": "no" can be used to specified specify pages excluded...

Copy link
Member

Choose a reason for hiding this comment

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

...specify pages to be excluded from searching...

@@ -45,7 +46,7 @@ Let's examine a typical `site.json` file:
| **baseUrl** | The base url relative to your domain. Default: <code></code>&nbsp;(empty). For example, if you are using Github Pages to host your deployed website and it is published at `https://markbind.github.io/site-demo-cs2103`, then your `baseUrl` would be `/site-demo-cs2103`. You can use this variable for [specifying path reference](contentAuthoring.html#specifying-path-reference) of images and links. **You may need to change the `baseUrl` when deploying to a different repo.** |
| **faviconPath** | The location of the favicon. Default: `favicon.ico`. If the favicon was recently changed, you may need to force-refresh to see the new image. |
| **titlePrefix** | The prefix for all page titles. The separator <code>-</code> will be inserted by MarkBind. |
| **pages** | An array of pages to be rendered. The `src` is the file; `title` is the page title for the generated web page. Titles specified here take priority over titles specified in the [front matter](contentAuthoring.html#front-matter) of individual pages. Alternatively, `glob` can be used to define a file pattern. If `title` is not specified in the front matter, the page will have `titlePrefix` as its title. |
| **pages** | An array of pages to be rendered. The `src` is the file; `title` is the page title for the generated web page. Titles specified here take priority over titles specified in the [front matter](contentAuthoring.html#front-matter) of individual pages. Alternatively, `glob` can be used to define a file pattern. If `title` is not specified in the front matter, the page will have `titlePrefix` as its title. `"searchable": "no"` can be used to specified pages to be excluded from searching. If you need to exclude pages covered in both page entry and glob entry, you need to specify `"searchable": "no"` in page entry.|
Copy link
Member

Choose a reason for hiding this comment

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

"searchable": "no" can be used to specified specify pages excluded...

Did you miss this one?

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Do squash your commits

@nusjzx nusjzx force-pushed the 269-allow-pages-excluded-from-search branch from 99d544d to 24fc5cf Compare July 2, 2018 08:23
@yamgent yamgent merged commit e6fdd1b into MarkBind:master Jul 3, 2018
@yamgent yamgent modified the milestones: v1.9.0, v1.9.0 (Migration), v1.8.4 Jul 3, 2018
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.

Allow some pages to be excluded from search index
4 participants