Fix current path detection for locale URL parameter#8792
Conversation
changelog: Bug Fixes, Localization, Fix tab navigation not showing selected tab in specific parameter combinations
| request.path == URI.parse(path).path | ||
| rescue URI::InvalidURIError | ||
| recognized_path = Rails.application.routes.recognize_path(path) | ||
| [recognized_path, request].pluck(:controller, :action).uniq.one? |
There was a problem hiding this comment.
This hazards toward being overly "clever" in a way which harms readability. Open to revising.
This is basically equivalent to the expanded form...
| [recognized_path, request].pluck(:controller, :action).uniq.one? | |
| request[:controller] == recognized_path[:controller] && | |
| request[:action] == recognized_path[:action] |
There was a problem hiding this comment.
Could also extract a method for some self-documenting code, but at that point we've probably lost any benefit of "simplifying"
| [recognized_path, request].pluck(:controller, :action).uniq.one? | |
| same_hash_values?([recognized_path, request], [:controller, :action]) | |
| end | |
| def same_hash_values?(hashes, properties) | |
| hashes.pluck(*properties).uniq.one? |
There was a problem hiding this comment.
How about:
| [recognized_path, request].pluck(:controller, :action).uniq.one? | |
| request.slice(:controller, :action) == recognized_path.slice(:controller, :action) |
There was a problem hiding this comment.
I think I've probably talked myself into just going with the simple version 😄
There was a problem hiding this comment.
@zachmargolis Oops, missed your comment 'til now. Yeah, that might be a reasonable compromise of conciseness and readability!
There was a problem hiding this comment.
@zachmargolis Unfortunately doesn't work as-is, since request isn't actually a hash, it's a ActionDispatch::Request (ActionDispatch::TestRequest in spec).
There was a problem hiding this comment.
ah dang. Thanks for trying, I do agree with the simpler approach that you landed on
| allow(vc_test_request).to receive(:path).and_return('/first') | ||
| let(:request_path) { '/first' } | ||
|
|
||
| class ExampleController < ApplicationController; end |
There was a problem hiding this comment.
the way RSpec uses blocks, class definitions like this end up at the global scope, so they can collide across specs, especially with a generic name like ExampleController
A few options:
- Much more specific controller name
TabNavExampleController - anonymous controller (
let(:example_controller_class) { Class.new(ApplicationController)- this probably wouldn't play well with the router :[
- Delete/Undef the controller class in after the block cleanup?
Object.remove_const(:ExampleController)
There was a problem hiding this comment.
Good call-out. I know RSpec creates its own namespace for each spec, so maybe we can tie into that? Dunno if there's a programmatic way, but maybe it's fine to do manually. Based on some quick local testing:
| class ExampleController < ApplicationController; end | |
| class RSpec::ExampleGroups::TabNavigationComponent::ExampleController < ApplicationController; end |
Having some trouble with actually getting the routing to respect this, though (capitalization, etc).
There was a problem hiding this comment.
Actually, since we don't care about running any code associated with the controller action, and since ViewComponent already assumes the presence of an ApplicationController, maybe it's fine to not define a new controller at all, and use that instead.
See 219bd88
2 participants
🛠 Summary of changes
Improves "current path" detection for TabNavigationComponent, fixing an issue where locale specified through a URL parameter would not be reflected.
Example: https://secure.login.gov/?locale=es
Related Slack discussion: https://gsa-tts.slack.com/archives/C0NGESUN5/p1689604911179219
📜 Testing Plan
👀 Screenshots