ckolderup / postmarks

a single-user bookmarking website designed to live on the Fediverse
https://postmarks.glitch.me
MIT License
452 stars 38 forks source link

Adjusted SQL REGEX to fix bookmark search #155

Closed TomCasavant closed 8 months ago

TomCasavant commented 8 months ago

PR to fix #152

Adjusted the REGEX for searching for tags %#${tag} % - matches on tags that contain the tag name and has a space at the end, so searching for the ai tag would match if it was "#ai #smarthome #etc" and "#smarthome #ai #etc", but not "#air #smarthome #etc" or "#email #air #etc" (the previous version would have matched on 'air' or 'email' because they contain 'ai')

%#${tag} - handles matches for the final tag in the string, same rules as above but does not have a space at the end.

The 2nd part of this issue is a little more complicated and I wanted to get your advice before working on it. Specifically, with it not highlighting tags if capitalization doesn't match, I fixed it on my site by adding a toLowerCase helper to handlebars and applying it to anywhere where it links to /tagged/tag so that all url paths are lowercase

    toLowerCase(a) {
      return a.toLowerCase();
    },

and then also changing ifIn and ifThisTag so that they compare ignoring the capitalization

    ifIn(item, array, options) {
      const lowercased = array.map(tag => tag.toLowerCase());
      return lowercased.indexOf(item.toLowerCase()) >= 0 ? options.fn(this) : options.inverse(this);
    },

     ifThisTag(tag, path, options) {
      return path  === `/tagged/${tag}`.toLowerCase() ? options.fn(this) : options.inverse(this);
    },

I didn't know if it was best practice or if you had an opinion on throwing these toLowerCase() functions everywhere, it feels wrong but the other option I think involves just lowercasing the tag before it gets passed into any page (which I don't think would be right, as I'd expect we'd want to maintain the capitalization when showing the user the tags underneath the bookmark

TomCasavant commented 8 months ago

If you want to split this into 2 separate issues and merge this one to fix the search issue we can do that as well

ckolderup commented 8 months ago

I think it's totally fine in the case of ifIn and ifThisTag but I'm missing why you need to change the URL generation / create a helper function for it. Isn't it enough to change the comparisons in ifIn and ifThisTag? (I wish I had caught those when we merged that PR!)

TomCasavant commented 8 months ago

I was thinking we had to do that in order to make the 'removeTag' work, e.g. if I am at /tagged/SmartHome/AI and I click remove tag on #smarthome it tries to remove /smarthome/ from the url which isn't in there.

But, I was just thinking about it and I just have to change the removeTag function to something like this:

    removeTag(tag, path) {
      return path
        .split('/')
        .filter((x) => x.toLowerCase() !== tag.toLowerCase())
        .join('/');
    }
ckolderup commented 8 months ago

yeah, nice! that should work well, and handle situations where people might have typed a URL manually or whatever else. I don't mind extra toLowerCase calls in JS proper-- I'm used to seeing them whenever string comparisons are being made. Avoiding introducing it to the render layer seems worthwhile for sure, though.

TomCasavant commented 8 months ago

Should be ready for review now