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

Now we take predefined search values from outer file #763

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

Altai-man
Copy link
Member

Plain text file because it's universal for all.

I've left js syntax here, because it's easy and we don't have to do anything else with source file.

I want opinions about - is it needed to change file to some format and then parse it in htmlify(if someone hates piece of js code in some outer file)? Maybe like this:

Category
Value
Url

Category
...

it is quite easy file format and I can implement this.

Ability to make links that point to just pages, not anchors, is quite good for search, for example, see #724

@zoffixznet
Copy link
Contributor

Personally I'm 👎 on having a human-generated list of search terms. Content changes and pages get deleted. This PR introduces another maintenance burden that we need to keep predefined-search-items up to date and accurate.

Is there no way to use the X<> POD marker to generate these search terms?

@Altai-man
Copy link
Member Author

@zoffixznet, afaik - no. But I'm not a pod6 magician, of course, so I can be wrong.

The thing is, we already have two hardcoded entries in js-code. I just provided an easier way to update them.
cast @AlexDaniel, @gfldex.

@zoffixznet
Copy link
Contributor

The thing is, we already have two hardcoded entries in js-code. I just provided an easier way to update them.

Ah, my bad. I missed that. It'd be best if we didn't have those, but if there's no way then there's no way.

@gfldex
Copy link
Contributor

gfldex commented Jul 26, 2016

X<> allowes # in the name but L<> doesn't. Technically we could do a uri_encode but that would require special casing in a few spots. Also the URL rewriter would get more complex. I prefer to have to the waterbed of complexity to be simple and in exactly one spot.

@AlexDaniel
Copy link
Member

I like it.

We can have a discussion about whether we should have manually defined items or not, but we already do, so… Basically I see no reasons not to merge.

The only thing is that people will now be tempted to add search items manually (given that now it is slightly easier). But that's OK, once there is a way to get rid of this file we will do it, but right now let's improve the search.

@gfldex gfldex merged commit ea3b332 into master Jul 27, 2016
@Altai-man Altai-man deleted the predefined_search branch July 31, 2016 17:24
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.

4 participants