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

fix: search does not find the contents of the table #1198

Merged
merged 2 commits into from Jun 7, 2020

Conversation

sy-records
Copy link
Member

Summary
The previous commit #1015 only fixed the situation where there was no content before the table. like this:

## Errno List

| C Name  | Value | Description       |
| ------- | ----- | ----------------- |
| Success | 0     | Success           |
| EPERM   | 1     | Operation not per |

image

But this one below is not searchable.

## Errno List

Here are some descriptions....

| C Name  | Value | Description       |
| ------- | ----- | ----------------- |
| Success | 0     | Success           |
| EPERM   | 1     | Operation not per |

image

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:


  • DO NOT include files inside lib directory.

@vercel
Copy link

vercel bot commented May 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/qfd7yuok4
✅ Preview: https://docsify-preview-git-fork-sy-records-fix-search.docsify-core.now.sh

@sy-records sy-records requested review from anikethsaha and a team May 27, 2020 11:10
@sy-records sy-records changed the title fix search: No table content found fix: search does not find the contents of the table May 27, 2020
Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

@sy-records sy-records changed the title fix: search does not find the contents of the table fix(search): search does not find the contents of the table May 27, 2020
@sy-records
Copy link
Member Author

Is now okay? @anikethsaha

@anikethsaha
Copy link
Member

remove the scope, search , currenlty we dont have scope.

Also, change the commit message as well.

Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

Good Job. 👍

Litte suggestion.
I think this block could refine.

 if (!token.text && token.type === 'table') {
          token.text = token.cells
            .map(function(rows) {
              return rows.join(' | ');
            })
            .join(' |\n ');
        }

        index[slug].body += '\n' + (token.text || '');
      } else {
        if (!token.text && token.type === 'table') {
          token.text = token.cells
            .map(function(rows) {
              return rows.join(' | ');
            })
            .join(' |\n ');
        }

        index[slug].body = index[slug].body
          ? index[slug].body + token.text
          : token.text;
      }

what the the difference is solving the index[slug].body.
it seems too much duplicate here.

if (!token.text && token.type === 'table') {
          token.text = token.cells
            .map(function(rows) {
              return rows.join(' | ');
            })
            .join(' |\n ');
        }

@Koooooo-7
Copy link
Member

and I prefer to use return in some cases direcly to avoid useing more if else branch.
Idk whether it should be applied to docsify.
WDYT ?

@sy-records sy-records changed the title fix(search): search does not find the contents of the table fix: search does not find the contents of the table May 28, 2020
@sy-records
Copy link
Member Author

sy-records commented May 28, 2020

@Koooooo-7 Is there a problem with using return in forEach?

tokens.forEach(token => {
if (token.type === 'heading' && token.depth <= depth) {
const { str, config } = getAndRemoveConfig(token.text);
if (config.id) {
slug = router.toURL(path, { id: slugify(config.id) });
} else {
slug = router.toURL(path, { id: slugify(escapeHtml(token.text)) });
}
index[slug] = { slug, title: str, body: '' };
} else {
if (!slug) {
return;
}
if (!index[slug]) {
index[slug] = { slug, title: '', body: '' };
} else if (index[slug].body) {
token.text = getTableData(token);
index[slug].body += '\n' + (token.text || '');
} else {
token.text = getTableData(token);
index[slug].body = index[slug].body
? index[slug].body + token.text
: token.text;
}
}
});
slugify.clear();
return index;
}

nah, it's okay.
I wonder how to make it simple(cuz there r too much if else branch.):100:
never mind.

@sy-records sy-records requested a review from a team June 7, 2020 08:12
@sy-records sy-records requested a review from trusktr June 7, 2020 08:17
@anikethsaha anikethsaha merged commit 31010e4 into docsifyjs:develop Jun 7, 2020
@anikethsaha
Copy link
Member

is there any issue which is being fixed by this PR ? feel free to close that one

@sy-records sy-records deleted the fix-search branch June 7, 2020 08:38
@sy-records
Copy link
Member Author

No relevant issue. I found the problem, fixed it by the way.

@anikethsaha
Copy link
Member

great

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.

None yet

3 participants