Change %20 in URLs to _

[spanishharlem]

I changed all the URLs according to #40. I decided it'll be the best idea to use _ as it's the same what's used on Wikipedia. The phrase sent to the Wikipedia API also replaced.

Sorry for mess with the commits, every time after I commited something, I found a flaw.

[waldyrious]

@Nalapl3 that was quick :) don't worry about the commits, they can be cleaned up at the merging step.

I think this might need some ironing out, though: try out a few multi-word searches at https://nalapl3.github.io/primerpedia/ -- for me, they're returning related articles, but not the primary ones I'd expect.

[spanishharlem]

This is now fixed, it actually happened also on your version. The thing is, you have to search for eg. "United_States", not "United%20States" to make it search the right article.

[waldyrious]

Huh, that's odd -- I never noticed that until now, but even when going back to the first commit that introduced search (fc77927) shows this behavior. Either I never noticed this, or perhaps there have been changes in the search backend that made the API return different result. In any case, thanks for fixing it :)

[waldyrious]

I guess it's better to merge this now to avoid further merge conflicts due to other PRs being opened and merged in the meantime. We can always make additional adjustment afterwards, if needed.

Unless someone asks otherwise, I'll rebase and merge once #42 is complete.

[waldyrious]

Ok, #42 is now merged; I am out of FOSS time for today, but tomorrow I should be able to tackle this one.

[waldyrious]

I've rebased the branch, fixing the conflicts and simplifying the commits and the code itself (the regexes, in particular, could be simplified to just translate between _, %20 and ). The diff of the PR should be easier to parse, too, as I cherry-picked commit 4bdb0bc directly into gh-pages, since it was supposed to be there anyway.

I tested the changes at https://nalapl3.github.io/primerpedia/, and they appear to be working as expected, but I'd prefer to have a second opinion, just in case I messed up some edge case. @Jan-Ka can you take a look?

[waldyrious]

I guess we should also replace %20 with _ in the url that gets copied to the clipboard now that #42 has been merged. Would you like to add that change to this branch, @Jan-Ka?

[Jan-Ka]

Sure thing @waldyrious !

Edit: I can't add the changes to this merge request, can I? Edit2: On a second glance - the link is cleaned of %20 on line 163. Did you mean something else?

[waldyrious]

Edit: I can't add the changes to this merge request, can I?

I believe you can. Since last year Github allows maintainers to edit PR branches.

Edit2: On a second glance - the link is cleaned of %20 on line 163. Did you mean something else?

Hm... If you test it at https://nalapl3.github.io/primerpedia/, does it work for you?

Actually, I found a separate error already: I can't do

var queryParam = getQueryVariable("search").replace(/_/g, ' ');

before

if(queryParam !== null) {

Otherwise I get a TypeError: getQueryVariable(...) is null. I'll have to fix that. Should I also change the null comparison to the more complete one you used elsewhere?

if(typeof queryParam === "string" && queryParam.length > 0) {

[Jan-Ka]

Thanks for the Link!

No, it does not work currently but because of the reasons you already mentioned. Otherwise I'm pretty certain that it will work as soon as the null check is introduced.

I think we can get away with

if(queryParam !== null) {

because the result of getQueryVariable can only ever be a string (ensured through decodeURIComponent()) or null and as long as it's a string replace() will work. Would probably be a good Idea to properly document the function with jsDoc as well.

/*
 *Get query string from URL parameter
 *@see [origin on stackoverflow]{@link https://stackoverflow.com/a/2091331/266309}
 *@param {string} parameter - name of the query parameter to retrieve
 *@returns {string|null} - decoded query parameter or null
 */
function getQueryVariable(parameter) {

[waldyrious]

@Jan-Ka I've updated the code but I guess I did something wrong because now in https://nalapl3.github.io/primerpedia/, if I search for a string (or load a random article) and then click the clipboard icon, what's copied is not the entire URL but just, e.g. /primerpedia/?search=John_Doe. Any clues about what change in this PR interfered with the clipboard feature?

[Jan-Ka]

@waldyrious to be honest I'm a tad confused. This also happens on the main branch since the function getShareableLink only uses location.pathnamewhich of course only contains /primerpedia. It's a straight forward fix to add location.origin as well but I'm fairly certain that I tested this functionality thoroughly.

Probably wanted to append it to https://github.com/waldyrious/primerpedia/blob/8dfcbcbd06fad82ad2a486907f520b59876c2910/primerpedia.js#L112

but forgot?

[waldyrious]

Let me try it on this branch; I'll sort out the history before merging.

Actually, could you make the change yourself? I'm not confident that I understand the flow of the browser history manipulation feature. (We probably need to think about documenting the code a bit more sometime soon.)

The change can be made directly on the browser: just click the "Files changed" tab on this PR and click the pencil button to edit the primerperdia.js file.

[Jan-Ka]

While trying to implement this features I found that it's not completely done with just adding window.location.origin (as it can start with // which would break the link). There needs some proper url handling (add if search is missing, change if it's available) in place which is easy enough to implement, but I think it is actually out of scope for this PR since it's a bug of #42 .

How should we handle this? I would say merge this, create new Bug in reference to #42 and fix within new PR.

[waldyrious]

Agreed. I'll cleanup the commits a bit and merge later today.

[waldyrious]

Branch merged. I melded commit c8de176 into 23510b8, since it was a hotfix, and cherry-picked d6a58a7 onto gh-pages after the merge node, since it was related, but not integral to this PR. Thanks again @Nalapl3 for takling this, and sorry for the messy/lengthy review process :sweat_smile: