40564: Added support for 'disabled' state for individual select options#40565
40564: Added support for 'disabled' state for individual select options#40565rybkinevg wants to merge 20 commits intomagento:2.4-developfrom
Conversation
|
Hi @rybkinevg. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
@magento run all tests |
|
@magento run Static Tests |
|
@magento run all tests |
There was a problem hiding this comment.
Pull request overview
Adds framework-level support for disabling individual <option> entries in Magento\Framework\Data\Form\Element\Select, enabling source models to mark specific options as non-selectable (e.g., via ['disabled' => true]).
Changes:
- Render
disabled="disabled"on<option>when the option array contains a truthydisabledflag. - Add unit coverage validating disabled option rendering for both plain options and options within an
<optgroup>.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/internal/Magento/Framework/Data/Form/Element/Select.php | Adds disabled="disabled" attribute rendering for per-option disabled state. |
| lib/internal/Magento/Framework/Data/Test/Unit/Form/Element/SelectTest.php | New unit tests covering disabled option rendering (including optgroups) across various disabled values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
lib/internal/Magento/Framework/Data/Test/Unit/Form/Element/SelectTest.php
Outdated
Show resolved
Hide resolved
|
@magento run all tests |
ihor-sviziev
left a comment
There was a problem hiding this comment.
Functional test failures look as a false positives, so approving
|
@ihor-sviziev, since PR is approved, what are the next steps? is there anything else required of me? |
|
@rybkinevg, no, the PR will be voted and if it gets enough likes by community 👍 - it will be chosen for delivery |
…ns in EditableMultiselect
|
@ihor-sviziev , I've added support for the disabled option state in the Should I also cover the disabled option state in the I also noticed that $this->_model = $testHelper->getObject(
Editablemultiselect::class, // <-- here
[
'_escaper' => $escaper,
'random' => $randomMock,
'secureRenderer' => $secureRendererMock
]
);Should this be corrected as well? |
|
@rybkinevg I think the best approach is to cover this case for each component individually, since they all follow the same logic. Could you please update it accordingly? |
…makes them all 'test-name' instead of the correct value)
…tion value & label
…lemultiselectTest' test
|
@ihor-sviziev , I've added test cases for disabled option state in the |
…vidual-select-options
|
@magentoe run all tests |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
lib/internal/Magento/Framework/Data/Test/Unit/Form/Element/MultiselectTest.php
Show resolved
Hide resolved
|
@magento run all tests |
Description (*)
Added support for the
disabledproperty on individual<option>elements within select form fields.If the
disabledkey is set totruein the option array it renders thedisabled="disabled"attribute accordingly.Fixed Issues (if relevant)
Manual testing scenarios (*)
Select & Multiselect elements
1) Modify the builtin source model:
Open the file
app/code/Magento/Directory/Model/Config/Source/Country.phpand modify thetoOptionArray()method to randomly disable some options:2) Clear cache
3) Navigate to admin configuration area
Go to
Stores->Configuration->General->General, expand theCountry Optionssection4) Verify the "Default Country" field (select)
Uncheck the "Use system value" and click on the dropdown to expand the options, some options should be disabled
5) Verify the "Allow Countries" field (multiselect)
Uncheck the "Use system value" and click on the dropdown to expand the options, some options should be disabled
Editablemultiselect element
1) Modify the builtin source model:
Open the file
app/code/Magento/Tax/Model/Rate/Source.phpand modify thetoOptionArray()method to randomly disable some options:2) Clear cache
3) Navigate to tax rule management
Go to
Stores->Tax Rules, click "Add New Tax Rule"4) Verify the "Tax Rate" field (editablemultiselect)
Some options should be disabled
Questions or comments
Current implementation treats all truthy values of the
disabledproperty as the disabled state:I am not sure, is this correct, or should we treat only explicitly true values as disabled state? For example:
Contribution checklist (*)