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

Add Documentation Search to Website #2796

Merged
merged 10 commits into from Jun 20, 2019

Conversation

Projects
None yet
4 participants
@sanssecours
Copy link
Member

commented Jun 18, 2019

This PR contains a rebased (and cleaned) version of pull request #2581.

hesirui and others added some commits Apr 3, 2019

@sanssecours sanssecours added this to the 0.8.27 milestone Jun 18, 2019

@sanssecours sanssecours referenced this pull request Jun 18, 2019

Closed

Enhance search #2581

@lgtm-com

This comment has been minimized.

Copy link

commented Jun 18, 2019

This pull request introduces 1 alert when merging 239ee5e into e551ccb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

jenkins build homepage please

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Seems like that this PR still destroys the webpage (at least for me). If I go to https://www.libelektra.org/home now, I only see the header and:


{{ 'APP.GLOBAL.TITLE' | translate}}

    {{ 'APP.NAV.BUTTON.MAIN.HOME' | translate }}
    {{ 'APP.NAV.BUTTON.MAIN.NEWS' | translate }}
    {{ 'APP.NAV.BUTTON.MAIN.CONVERSION' | translate }}


It seems like https://www.jsdelivr.com/ is suddenly required for anything to work. Any idea why this is the case?

And even with jsdelivr, the search field does not contain text but {{'APP.ENTRIES:SEARCH:SEARCHBOX.PLACEHOLDER' | translate }}

So something is very fishy here.

@hesirui @s-pace any idea?

@hesirui

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Thank you very much for this detailed analysis!

Probably because that parsing happens in the searchfunction, I disabled?

Can you fix this please? If there is no other way without localization.

For developing I only build the frontend locally, unfortunately I cannot build the whole website as yajl (which is needed for the backend) doesn't work on OS X Mojave (I tried countless fixes suggested on stackoverflow, none worked).

Can you create an issue (in Elektra) for that please? Maybe @sanssecours knows some workaround.

@s-pace is there any way for better integration into Angular?

@hesirui

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Sure, I will fix the label of the search box.

</script>
<script type="text/javascript" src="https://cdn.jsdelivr.net/npm/docsearch.js@2/dist/cdn/docsearch.min.js"></script>

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 18, 2019

Contributor

For me this line looks like it could cause the problem I have with cdn.jsdelivr.net (destroying the whole homepage).

This comment has been minimized.

Copy link
@hesirui

hesirui Jun 18, 2019

Contributor

Maybe docsearch should be added in the package.json, if they provide an npm package?

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 18, 2019

Contributor

I do not know if this would fix the problem. @Namoshek can you maybe take a look?

This comment has been minimized.

Copy link
@Namoshek

Namoshek Jun 18, 2019

Contributor

For me the website works fine. I guess one of your anti-tracker browser extensions blocks the cdn?. I suggest adding docsearch to package.json and linking it in the concat.vendor target of our Gruntfile.js to pack it with the other vendor packages in a neat bundle:

concat: {
options: {
banner: dstFileBanner
},
vendor: {
src: [
'public/assets/skin/bootstrap/bootstrap.min.css',
'node_modules/highlight.js/styles/github.css',
'node_modules/angular-ui-notification/dist/angular-ui-notification.min.css',
'node_modules/ng-tags-input/build/ng-tags-input.min.css',
'node_modules/ng-tags-input/build/ng-tags-input.bootstrap.min.css',
'node_modules/angular-typewriter/npm-dist/angular-typewrite.css'
],
dest: 'public/assets/skin/vendor.css',
},
pacejs: {
src: ['node_modules/pace-progress/pace.min.js'],
dest: 'public/vendor/pace.min.js'
},
pacecss: {
src: ['node_modules/pace-progress/themes/blue/pace-theme-minimal.css'],
dest: 'public/vendor/pace.min.css'
}
},

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 18, 2019

Contributor

Thank you for the quick help!

Yes, it is caused by blocking the site. But in any case it should not make the whole site fail (it is obviously ok that the search will not work).

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

The "Go" button next to the search bar also does not seem to work.

@sanssecours

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

…unfortunately I cannot
build the whole website as yajl (which is needed for the backend)
doesn't work on OS X Mojave (I tried countless fixes suggested on
stackoverflow, none worked).

Maybe @sanssecours knows some workaround.

As far as I can tell the YAJL plugin works fine on my machine (macOS 10.14.5). We also test the YAJL plugin on macOS Mojave:

image: mojave-xcode-10.1

in all of the Cirrus CI Mac build jobs, where it seems to work fine too.

@hesirui

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

…unfortunately I cannot
build the whole website as yajl (which is needed for the backend)
doesn't work on OS X Mojave (I tried countless fixes suggested on
stackoverflow, none worked).

Maybe @sanssecours knows some workaround.

As far as I can tell the YAJL plugin works fine on my machine (macOS 10.14.5). We also test the YAJL plugin on macOS Mojave:

image: mojave-xcode-10.1

in all of the Cirrus CI Mac build jobs, where it seems to work fine too.

I am getting the error when I try to perform the npm install, I can give you the detailed error later.

@hesirui

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

The "Go" button next to the search bar also does not seem to work.

I can remove that button, with docsearch it doesn't have any function anyway, as the user has to click on a link in the autocomplete.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

I am getting the error when I try to perform the npm install, I can give you the detailed error later.

Yes but please in a new issue.

I can remove that button, with docsearch it doesn't have any function anyway, as the user has to click on a link in the autocomplete.

Yes, this makes sense.

@markus2330 markus2330 requested a review from Namoshek Jun 18, 2019

@Namoshek

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

And even with jsdelivr, the search field does not contain text but {{'APP.ENTRIES:SEARCH:SEARCHBOX.PLACEHOLDER' | translate }}

The reason for this most likely is that the docsearch plugin replaces the whole <input> element and angular does not evaluate expressions of elements added to the DOM after initial rendering. This can be done manually somehow.

Easier could be to manually translate the placeholder string and set it on the DOM element after docsearch has been initialized. Not sure if there is an afterInitialization event or something though.

@hesirui

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@sanssecours
Please include my last commits from the old pull request. I didn't know how I can push my changes into this one, as it is on your repo.

I fixed the translation, included docsearch in the package.json and added it in the gruntfile, removed the search button. I did not however merge all JS packages into one, as I didn't wanna restructure the whole frontend and possibly break something again, as I don't have much time left until the request has to be merged.

hesirui added some commits Jun 18, 2019

@sanssecours

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

I didn't know how I can push my changes into this one, as it is on your repo.

You can just use the “command line instructions” shown by GitHub to create a local copy of this branch:

git checkout -b sanssecours-🔍 master
git pull https://github.com/sanssecours/elektra.git 🔍

. Afterwards you can update and push to the resulting branch, just as you do normally.

@sanssecours

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

jenkins build homepage please

1 similar comment
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

jenkins build homepage please

@markus2330 markus2330 merged commit 5dddf7e into ElektraInitiative:master Jun 20, 2019

12 of 14 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 58.114%
Details
elektra-homepage Build finished. No test results found.
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Amazing, finally we have the doc search! Good job, the homepage now always work for me (if JavaScript from libelektra.org is accepted).

@sanssecours sanssecours deleted the sanssecours:🔍 branch Jun 20, 2019

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.