Fix label for attribute when not scoped to model#3696
Conversation
🦋 Changeset detectedLatest commit: 4a06e61 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the label for attribute assignment when the scope_id_false: false option is used. The fix ensures that the label's for attribute is properly set to match the input's ID after the ID has been finalized during initialization.
Key changes:
- Moved label
forattribute assignment to later in the initialization process - Added test coverage for the fixed behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/lib/primer/forms/dsl/input.rb | Moves the @label_arguments[:for] = id assignment from early initialization to after ID processing is complete |
| test/lib/primer/forms/input_test.rb | Adds test assertions to verify label for attributes are correctly set in existing test cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
myabc
force-pushed
the
fix/label-for-not-scoped-to-model
branch
from
9130190 to
7b26e33
Compare
This requires an upstream fix. However, given the potential to cause accessibility problems and broken UX (e.g. focus), we should track the current and desired/fixed behavior downstream. See primer/view_components#3696 See primer/view_components#3695
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
siddharthkp
requested review from
jonrohan
and removed request for
siddharthkp
|
Passing over to @jonrohan who would know more about this than me |
|
Just a quick note: I've kept this PR narrow in scope (forgive the pun) and tried to solve this in the "least invasive" way possible. However, there are other issues (such as namespacing - see #3698) for which I'm trying to find a more robust solution. |
This requires an upstream fix. However, given the potential to cause accessibility problems and broken UX (e.g. focus), we should track the current and desired/fixed behavior downstream. See primer/view_components#3696 See primer/view_components#3695
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
| @input_arguments = system_arguments | ||
| @input_arguments.delete(:id) unless @input_arguments[:id].present? | ||
| @label_arguments = @input_arguments.delete(:label_arguments) || {} | ||
| @label_arguments[:for] = id if id.present? |
There was a problem hiding this comment.
This was calling def id below which is the same as @input_arguments[:id] curious how this is different than what was already in place
There was a problem hiding this comment.
@jonrohan sorry for only just getting back to you. The order here is important. We should defer setting @label_arguments[:for] until after these lines, which can mutate @input_arguments[:id]:
unless @input_arguments.delete(:scope_id_to_model) { true }
@input_arguments[:id] = @input_arguments.delete(:id) { name }
end
|
Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days. |
Defers assigning label `for` value to later in `initialize` method, ensuring the `scope_id_false: false` option is respected. Closes primer#3695
myabc
force-pushed
the
fix/label-for-not-scoped-to-model
branch
from
b6b4909 to
e843dd2
Compare
Setting `for: nil` made Rails skip generating the scoped id, dropping the label association on the default model-scoped path. Sets `for` only when the input id is present, restoring Rails autogeneration otherwise. Adds a default-scope regression test and corrects the changeset to name the real `scope_id_to_model` option.
myabc
force-pushed
the
fix/label-for-not-scoped-to-model
branch
from
571d5dc to
4a06e61
Compare
What are you trying to accomplish?
Defers assigning label
forvalue to later ininitializemethod, ensuring thescope_id_false: falseoption is respected.Integration
It may require updating calling code that relies on the incorrect behavior.
List the issues that this change affects.
Closes #3695
Risk Assessment
Merge checklist