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

FLUID-5722: Add search component. #147

Merged
merged 9 commits into from Jun 30, 2019

Conversation

Projects
None yet
2 participants
@the-t-in-rtf
Copy link
Contributor

commented Apr 30, 2019

Adds search functionality based on Fuse.js. See FLUID-5722 for details.

Rather than commit a copy of another dependency, I converted nearly all dependencies to proper npm dependencies that are copied to the output in the "writeAfter" stage. The only dependency I could not clean up is the forked and modified highlight.js CSS, which is older than their packaged versioning available via npm.

cc: @amb26

the-t-in-rtf added some commits Apr 30, 2019

FLUID-5722: Add search functionality.
NOJIRA: Remove all "forked" local copies of dependencies in favour of proper dependencies.
@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@amb26, the new search has been manually tested in Safari, Opera, Chrome, Firefox, IE 11, and Edge.

There were some small problems in pinning to a non-global docpad in Windows, I had to avoid using npm run directly on the docpad binary, but the new dependency management, etc. is verified working in a Windows VM as well.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@amb26 and I have discussed the limitations of the Fuse.js search WRT quoted words and phrases. We have agreed to address this later, but I wrote up my thoughts in FLUID-6378.

For this pass, we discussed adding gpii-binder and gpii-location-bar so that searches are navigable (back button) and persistable (bookmarking, copying, etc.). We also discussed adding _target attributes to search hit links.

@the-t-in-rtf the-t-in-rtf changed the title FLUID-5722 FLUID-5722: Add search component. May 2, 2019

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@amb26, I just added browser tests of the search component. The new URL persistence should allow me to add a search form from the sidebar, which I will do shortly, and then this will be ready for further review.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@amb26 this now includes the "mini search" for non-search pages. I also fixed the heading highlighting for the search page so that the search link is now correctly highlighted if you're on the search page.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@amb26, I'd like to include the security updates from this pull in with this: GPII/gpii-location-bar-relay#5

That should be a quick review, and should be completed before we merge this one.

@the-t-in-rtf

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@amb26, this now includes the security updates for gpii-location-bar-relay, and should be ready for further review.

@amb26

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

Thanks, this is a brilliant set of improvements, as well as the totally useful headline functionality, we have also ditched the copy-pasted ancient half-Infusion, as well as getting the npm scripts to run properly under Windows. Totally great

@amb26 amb26 merged commit 0b48485 into fluid-project:master Jun 30, 2019

1 check passed

license/cla Contributor License Agreement is signed.
Details
@amb26

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

Unfortunately the "deploy" script doesn't seem to run (any more), although I suspect this is not the result of work in this branch. The failure occurs within the plugin during the actions

        gitCommands = [['git', 'init'], ['git', 'add', '--all', '--force'], ['git', 'commit', '-m', opts.lastCommit], ['git', 'push', '--quiet', '--force', opts.remoteRepoUrl, "master:" + config.deployBranch]];
        return safeps.spawnMultiple(gitCommands, {

which then causes git to complain

info: log Performing push...
Initialized empty Git repository in /cygdrive/e/Source/gits/infusion-docs/out/.git/

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

Perhaps the script needs to have cwd set correctly or some such shiznitz.

@amb26

This comment has been minimized.

Copy link
Member

commented Jun 30, 2019

I was able to get the deploy action completed by simply pasting into the console the git commands that the plugin attempted to execute. I guess I'll just have to accept that some quirk of my installation prevents the plugin from finding "the right" git ....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.