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

Parameterize Category labels in paths instead of escaping them #2

Closed
morgant opened this issue Apr 20, 2022 · 4 comments
Closed

Parameterize Category labels in paths instead of escaping them #2

morgant opened this issue Apr 20, 2022 · 4 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@morgant
Copy link
Collaborator

morgant commented Apr 20, 2022

In the initial implementation of Comfy::Archive::IndexController#index I was lazy and just used CGI::unescape so that it was an easy #for_category query. The view just does a CGI::escape on the Comfy::Cms::Category#label (from Comfy::Archive::Index#categories) to encode them, but that results in paths like /blog/category/In+the+News. The labels should really be encoded with String#parameterize to be consistent with how Rails & ComfortableMexicanSofa build paths & URLs.

@morgant morgant self-assigned this Apr 20, 2022
@morgant morgant added the enhancement New feature or request label Apr 20, 2022
@morgant morgant added this to the 0.3.0 milestone Apr 20, 2022
@morgant
Copy link
Collaborator Author

morgant commented Apr 20, 2022

My thought is to approach this one of two ways:

  • Add a Comfy::Archive::IndexController method to find matching Comfy::Cms::Categories by fetching all of the categories, then doing a basic Array#find on the parameterized label.
  • Extend the Comfy::Cms::Category to add a slug field, like Comfy::Cms::Page has, and then query by that in Comfy::Archive::IndexController.

The first seems easiest to maintain, but may not be particularly performant on sites with lots of categories. The latter would be far more performant, but I'm a little wary of adding & maintaining additional columns for models in ComfortableMexicanSofa.

Either way, I should make this optional via a config option to ease transitions for anyone relying on the current implementation (esp. since ComfyBlog seems to have the same issue). I should also add some helper methods for generating the appropriate year/month & category archive links to simplify the views a bit.

morgant added a commit that referenced this issue Apr 21, 2022
…chive::IndexController to use either current escaped Comfy::Cms::Category labels or new parameterized Category labels, plus tests. Now also results in a 404 page if the Category doesn't exist. Issue #2
morgant added a commit that referenced this issue Apr 21, 2022
…rchive_link_to_category' methods, the later automatically generating the appropriately encoded category label, plus tests. Issue #2
@morgant
Copy link
Collaborator Author

morgant commented Apr 21, 2022

I went with the simpler implementation and we can optimize later, as necessary. I have also added a new Comfy::ArchiveHelper with date & category link generation methods, the latter of which utilizes the new ComfyArchive.config.parmeterize_category config option.

morgant added a commit that referenced this issue Apr 21, 2022
… of 404 pages if Comfy::Cms::Category doesn't exist is optional, plus tests. Issue #2
@morgant
Copy link
Collaborator Author

morgant commented Apr 21, 2022

My implementation also added other new functionality, specifically that the 404 error page is returned (or an exception thrown) if a matching Comfy::Cms::Category cannot be found. However, I realized that we're actually taking advantage of the old functionality internally, so I've added a new ComfyArchive.config.strict_categories option (defaults to true) to enable said new functionality, that we we can continue to abuse it.

@morgant
Copy link
Collaborator Author

morgant commented Apr 22, 2022

All tests pass and this is working well in our internal testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant