Add searchbar component by nusjzx · Pull Request #28 · MarkBind/vue-strap

nusjzx · 2018-05-23T04:33:40Z

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:

Related MarkBind/markbind#162, MarkBind/markbind#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)
highlightAndExtract function will extract the matching results then highlight the phrases.
for jumping to a specific heading, we have one entry for each heading(suggested by Prof Damith).
Thus, Instead of processing on a single string that combines title, keywords and headings, we match headings first, then title and keywords.

Testing instructions:

in vue-strap npm run build
copy paste vue-strap.min.js to markbind
After cloning the CS3281 website, go to common/header.md, and replace the use of with :
test on 3281 website

…62-search-for-headings

acjh · 2018-05-23T04:52:26Z

Resolves #162 and #204

Change to:

Related: MarkBind/markbind#162, MarkBind/markbind#204

Use "Resolves" if only need to upgrade vue-strap.min.js, i.e. no corresponding change is required.

nusjzx · 2018-05-23T05:18:01Z

I add a searchbar.vue which will change the vue-strap.min.js. @acjh

acjh · 2018-05-23T05:20:47Z

How is that relevant?

nusjzx · 2018-05-23T05:23:38Z

what do you mean by upgrade vue-strap.min.js

acjh · 2018-05-23T05:30:06Z

I mean upgrade vue-strap.min.js in the MarkBind/markbind repository.

If corresponding change is required, then this PR alone does not resolve the issues.
If you use the Resolves keyword, then merging this PR would incorrectly close them.

nusjzx · 2018-05-23T05:31:45Z

Ok, get it

yamgent · 2018-05-23T06:42:22Z

src/Typeahead.vue

  filters: {
    highlight (value, phrase) {
-      return value.replace(new RegExp('(' + phrase + ')', 'gi'), '<strong>$1</strong>')
+        return value.replace(new RegExp('(' + phrase + ')', 'gi'), '<mark>$1</mark>')


The indentation here is not necessary (we use 2 spacing per level).

@nusjzx This change is not used right?

It is used here in the associated markbind's PR, together with the new filter highlightAndExtract:

https://github.com/nusjzx/markbind/blob/56f8b2dd950aa628d0de332cab6d1cacdcf597dd/asset/js/setup.js#L24-L25

The reason is we do the highlighting differently for headings and for keywords.

Headings: Keep everything
Keywords: Only the matching keyword is conserved, with the matching characters highlighted.

As mentioned last week, shall we create a mark filter instead?

yamgent · 2018-05-23T07:02:00Z

src/searchbar.vue

+              var query = this.value.toLowerCase()
+              var hasHeadingMatch = false
+              var title = value.title.toLowerCase()
+              var keywords = value.keywords? value.keywords.toLowerCase(): ""


Need to have spacing before the ? and :. I.e.

value.keywords ? value.keywords.toLowerCase() : ""

Also we use single quotes for strings, '' rather than "".

Since this is a new file, let's use eslint.
Let me see if I can configure it. For now, you can copy-paste it into a .js file in MarkBind.
We can extend this to all custom files (components) later.

yamgent · 2018-05-23T07:03:36Z

src/searchbar.vue

+              var src = value.src
+              if (Object.keys(headings).length !== 0) {
+                for (var key in headings) {
+                	var heading = headings[key].toLowerCase();


Indentation seems to be off here.

yamgent · 2018-05-23T07:33:16Z

src/searchbar.vue

+            var matches = []
+            var words
+            this.data.map(value => {
+              var query = this.value.toLowerCase()


Instead of having to convert to lower case toLowerCase(), maybe we can use search() instead of index()?

So instead of: heading.indexOf(query), we use heading.search(new RegExp(query, "i"))). The "i" will make the search case-insensitive.

Hopefully this will make the code clearer.

@nusjzx Is it necessary to modify primitiveData?

acjh · 2018-05-23T04:37:45Z

src/index.js

 import tipBox from './TipBox.vue'
 import trigger from './trigger.vue'
 import typeahead from './Typeahead.vue'
+import searchbar from './Searchbar.vue'


Move above import tipBox

acjh · 2018-05-23T04:44:38Z

src/index.js

  trigger,
-  typeahead
+  typeahead,
+  searchbar


Move above trigger

acjh · 2018-05-23T07:38:58Z

src/Typeahead.vue

  filters: {
    highlight (value, phrase) {
-      return value.replace(new RegExp('(' + phrase + ')', 'gi'), '<strong>$1</strong>')
+        return value.replace(new RegExp('(' + phrase + ')', 'gi'), '<mark>$1</mark>')


@nusjzx This change is not used right?

acjh · 2018-05-23T07:41:04Z

src/searchbar.vue

+            var matches = []
+            var words
+            this.data.map(value => {
+              var query = this.value.toLowerCase()


@nusjzx Is it necessary to modify primitiveData?

acjh · 2018-05-23T07:59:37Z

src/searchbar.vue

+              var query = this.value.toLowerCase()
+              var hasHeadingMatch = false
+              var title = value.title.toLowerCase()
+              var keywords = value.keywords? value.keywords.toLowerCase(): ""


Since this is a new file, let's use eslint.
Let me see if I can configure it. For now, you can copy-paste it into a .js file in MarkBind.
We can extend this to all custom files (components) later.

nusjzx · 2018-05-23T08:59:15Z

@acjh 1. the change is used, as we still need highlight filter
2. if prof want each headings each entry, we cannot process on the string only, we need the object

yamgent · 2018-05-23T09:03:03Z

src/searchbar.vue

+            this.data.map(value => {
+              var query = new RegExp(this.value, 'i')
+              var hasHeadingMatch = false
+              var title = value.title


Since we have gotten rid of the .toLowerCase(), the assignment might no longer be necessary, just use value.title below. E.g.:

matches.push({'title': value.title,

Same thing for keywords and headings.

yamgent · 2018-05-23T09:03:33Z

src/searchbar.vue

+              var query = new RegExp(this.value, 'i')
+              var hasHeadingMatch = false
+              var title = value.title
+              var keywords = value.keywords ? value.keywords : ''


You can use || instead. I.e.:

const keywords = value.keywords || '';

yamgent · 2018-05-23T09:04:20Z

src/searchbar.vue

+                for (var key in headings) {
+                  var heading = headings[key]
+                  if (heading.search(query) !== -1) {
+                    hasHeadingMatch = true


hasHeadingMatch -> hasMatchingHeading

yamgent · 2018-05-23T09:10:12Z

src/searchbar.vue

+        primitiveData() {
+          if (this.data) {
+            var matches = []
+            var words


Remove this declaration, declare this below when it is initialized, so it is something like:

const words = keywords ? keywords + title : title

yamgent · 2018-05-23T09:12:00Z

src/searchbar.vue

+            var matches = []
+            var words
+            this.data.map(value => {
+              var query = new RegExp(this.value, 'i')


if the variable does not change its value after initialization, you should use const instead of var:

const query = new RegExp(this.value, 'i')

Same for the rest of the variables below.

yamgent · 2018-05-23T09:18:04Z

src/index.js

  tooltip,
  pic,
-  trigger,
+	searchbar,


The indentation here is tab rather than space. I think your IDE might be set to tabs rather than space?

nusjzx added 5 commits May 18, 2018 13:02

Change style of highlight

b7d751a

Add searchbar to extract the matching search result only

18e009b

Change the data structure for jumping to specific heading

44f63d1

Merge branch 'master' of https://github.com/MarkBind/vue-strap into 1…

c05aa83

…62-search-for-headings

Add new line at EOF of searchbar.vue

58628a1

nusjzx mentioned this pull request May 23, 2018

Support searching for headings MarkBind/markbind#240

Merged

yamgent suggested changes May 23, 2018

View reviewed changes

yamgent changed the title ~~162 search for headings~~ Add searchbar component May 23, 2018

acjh reviewed May 23, 2018

View reviewed changes

nusjzx added 2 commits May 23, 2018 16:46

Fix indentation and use search instead of indexOf

04684f2

Order import alphabetically

d12aa29

yamgent suggested changes May 23, 2018

View reviewed changes

yamgent reviewed May 23, 2018

View reviewed changes

acjh mentioned this pull request May 23, 2018

Add eslint #29

Merged

Fix coding style

ab6bd27

nusjzx closed this May 23, 2018

nusjzx deleted the 162-search-for-headings branch May 23, 2018 12:22

yamgent mentioned this pull request May 24, 2018

Add searchbar component #30

Merged

Conversation

nusjzx commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acjh commented May 23, 2018

Uh oh!

nusjzx commented May 23, 2018

Uh oh!

acjh commented May 23, 2018

Uh oh!

nusjzx commented May 23, 2018

Uh oh!

acjh commented May 23, 2018

Uh oh!

nusjzx commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamgent May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nusjzx commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamgent May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nusjzx commented May 23, 2018 •

edited

Loading

yamgent May 23, 2018 •

edited

Loading

yamgent May 23, 2018 •

edited

Loading