Support searching for headings

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.

[nusjzx]

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

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

Resolves#162 and #204

What is the rationale for this request?
highlighting and extracting the search result helps users to identify the results they want easily,
searching for headings and jump to the headings on click helpers users to go to website sections easily.

What changes did you make? (Give an overview)
Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting
while the template is changed in setup.js

jumping to headings: siteData now includes headings also, collectHeadings collect h1, h2, h3 headings, and put them in an object where the key is the id , value is the text content. Jumping is done by adding a anchor after the url.

Testing instructions:

1. in vue-strap npm run build
2. copy paste vue-strap.min.js to markbind
3. test on 3281 website

[acjh]

Can you add some screenshots?

[yamgent]

Missing step for testing instruction: After cloning the CS3281 website, go to common/header.md, and replace the use of <typeahead> with <searchbar>:

<searchbar:data="searchData" placeholder="Search" :template="titleTemplate" template-name="title" :on-hit="searchCallback"></searchbar>

[nusjzx]

Screenshot:

[acjh]

Is that generating multiple entries for a single page? Doesn't seem right.
Also, are long headings truncated?

[nusjzx]

long headings are not truncated, as we have each heading per entry

[nusjzx]

prof just checked the effects, this is what he suggested

[yamgent]

Good work implementing the search for headings. Some comments about the commit titles:

• Add headings in search data -> Page.js: Collect headings for siteData.json

• Highlight matches in search result -> Search: Modify dropdown to highlight heading matches

• Use searchbar to replace typeahead -> Search: Replace typeahead with searchbar

• Update new syntax for searchbar in userguide -> Docs: Replace typeahead with searchbar

The commit "Merge branch 'master' of https://github.com/MarkBind/markbind into 16…" can be gotten rid of by rebasing the branch to the latest master.

Also some suggestions for improvements:

[yamgent]

How about:

constpage=`${baseUrl}/${match.src.replace('.md','.html')}`;constanchor=(Object.keys(match.heading).length===0) ? '' : `#${match.heading.id}`;window.location=`${page}${anchor}`;

[yamgent]

Extra blank line

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[damithc]

prof just checked the effects, this is what he suggested

I suggested one entry per heading because otherwise there is no way to jump to a specific heading of a page (unless we modify the UI of the typehead component drastically). Need to dogfood this version over cs2103 website to see if the current version is user-friendly. @nusjzx try using real search scenarios over cs2103 website to gauge usability.

[yamgent]

Minor nits

[yamgent]

@nusjzx This is still not resolved :P. This addition of blank line is not necessary.

[yamgent]

Shouldn't delete this line

[yamgent]

This should use forEach rather than map so that we can avoid the return 0;

[yamgent]

Sorry, the correct name should be each() for this case.

[damithc]

Noticed that in your staged version the drop-down of the nav bar is too long.

[nusjzx]

@damithc noted

[yamgent]

This PR needs to be rebased to the latest master, there's a conflicting file.

[danielbrzn]

Looked through the PR and the live demo available at https://nusjzx.github.io/website-base/.

It's a good start to making the search more functional and user-friendly, especially given that important headings might be hidden behind panels that the user has to click in order to see.

Some comments:

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

• There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

[yamgent]

There's a noticeable delay between the user typing a query, and having the query + results show up, which doesn't make for a very good user experience. You can observe how the results are instantly served here. The delay could be due to the highlighting of the match, or because the data source is large (948 pages in siteData.json).

I highly suspect it is mainly due to the large data source. If the highlighting of the match is a problem then it also means we are also rendering too many items in the list.

Two things we could possibly do to make this less of an issue:

• Load the data in the background, and show a loading spinner in the UI while it is loading. While it doesn't reduce the delay, it gives a visible feedback to the user of what is going on.
• When processing the list items one by one, if an item is matched already, then it can be shown on the screen first while we process the rest of the items (rather than the current implementation where we have to process everything before we show everything in one go).

[damithc]

Performance improvements can be done as separate PRs if appropriate.

[yamgent]

I think this iteration is good.

• Perhaps bolding the matches between the user's query and results might be a better experience for the user instead of highlighting the matches. It might seem a bit jarring to users.

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

Changes to the original component in MarkBind/vue-strap#30 made this approval no longer valid. I shall wait till the original component gets approved first before getting this re-evaluated.

[damithc]

Hmm... I personally find it OK actually, though I guess this is quite subjective, so if Prof finds the style too weird we can definitely change it.

We'll keep this point in mind. Can tweak later if needed.

[acjh]

implementation of highlighting and extracting is in MarkBind/vue-strap#28

Please change this to:

Follows MarkBind/vue-strap#30 — implementation of highlighting and extracting

And update this PR where required.

[yamgent]

Can remove :template="titleTemplate" and template-name="title" since we now have defaults.

[acjh]

Remove additional space in "␣␣:on-hit

[acjh]

Alphabetical order

[acjh]

+ .then(result => this.collectHeadings(results))
.then((result) => {

[acjh]

• Move this above collectIncludedFiles.
• Add jsdoc.

[acjh]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

[yamgent]

Records all h1 to h3 headings id-text pair into this.headings

Mentioning id-text pair would reveal too much details imo.

[acjh]

Should be renderedPage.

/** * Records h1,h2,h3 headings into this.headings * @param renderedPage a page with its headings rendered */Page.prototype.collectHeadings=function(renderedPage){

[yamgent]

@danielbrzn Mind taking a look at this again, specifically the code? Thanks! :)

[acjh]

Possibly a different PR:
A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

h1,h3,h4 (current)	h1,h2,h3

[acjh]

@nusjzx Note the change in the first line, from #240 (comment):

/***Recordsh1,h2,h3headingsintothis.headings

[nusjzx]

noted

[damithc]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

Alternatively, we can include h4 in search index.

[acjh]

h4 seems noisy:

[yamgent]

A handful of headings (e.g. Glyphicons) in MarkBind docs use h4, which can't be searched.
Should we "upgrade" those? The current headings look pleasant.

I think it is good to "upgrade" them.

If the new headings doesn't look pleasant, then I think the root of the issue is that the style for different headings are not distinct enough from each other, and we should fix that rather than skipping headings.

[yamgent]

@yamgent The changes in setup.js make this PR a breaking change, to be noted in the next release.

@acjh if it is a breaking change, should that make the version become 2.0.0? According to the semantic versioning scheme, since our new setup.js in this PR breaks existing websites if the authors do not make any changes to their HTML code on their navbar, it qualifies as not having a backwards-compatible feature.

[acjh]

A minor version will do. 1.x is still not production-ready for now.
We can follow those guidelines when we are ready for 2.0.