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

feat(ui5-select): Add support for disabled select options #2730

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

kineticjs
Copy link
Contributor

Allow the application specify select options as disabled. Disabled options are non-interactable.

Fixes: #2559

Allow the application specify select options as disabled. Disabled options are non-interactable.
@kineticjs kineticjs marked this pull request as ready for review January 27, 2021 03:34
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed one issue after testing a bit. If you have a list item with both selected and disabled initially and you press TAB to move the focus from the button (in this case) to the List, the focus is somehow lost, the arrows can't be used to move the focus between the items, and the disabled item gets the default browser outline

        <ui5-button>click</ui5-button>
	<ui5-list>
		<ui5-li>a</ui5-li>
		<ui5-li disabled selected >b</ui5-li>
		<ui5-li>c</ui5-li>
		<ui5-li>d</ui5-li>
	</ui5-list>

@kineticjs
Copy link
Contributor Author

kineticjs commented Jan 27, 2021

Yes, the logic for focusing the items had to take disabled items into account. I made an update. Any improvement suggestions on top if it are welcome..

Btw earlier I noticed another issue with disabled list-items (when the list is not used within a select) but couldn't find a proper solution to it (using the means of the framework) so didn't address it so far (btw I also wasn't sure if we need to handle that case):

        <ui5-list>
		<ui5-li>a</ui5-li>
		<ui5-li>b</ui5-li>
	</ui5-list>

=> After the list is rendered, its first item has tabindex 0.
Now, if we set "disabled" to "true" for the first item (from the console)
=> I see the first item is invalidated and correctly rerendered with tabindex -1,
but the second item still remains with tabindex -1
=> all list items now have tabindex -1

I see that the issue does not occur when the list is in a select and we disable select-options (rather than directly list-tems),
because the select causes the list to be invalidated as well.

Should we try to invalidate the list in the above case too? .. or is it outside our scope? Any suggestions/improvements you consider related are welcome!

@ilhan007
Copy link
Member

ilhan007 commented Feb 1, 2021

Yes, the logic for focusing the items had to take disabled items into account. I made an update. Any improvement suggestions on top if it are welcome..

Btw earlier I noticed another issue with disabled list-items (when the list is not used within a select) but couldn't find a proper solution to it (using the means of the framework) so didn't address it so far (btw I also wasn't sure if we need to handle that case):

        <ui5-list>
		<ui5-li>a</ui5-li>
		<ui5-li>b</ui5-li>
	</ui5-list>

=> After the list is rendered, its first item has tabindex 0.
Now, if we set "disabled" to "true" for the first item (from the console)
=> I see the first item is invalidated and correctly rerendered with tabindex -1,
but the second item still remains with tabindex -1
=> all list items now have tabindex -1

I see that the issue does not occur when the list is in a select and we disable select-options (rather than directly list-tems),
because the select causes the list to be invalidated as well.

Should we try to invalidate the list in the above case too? .. or is it outside our scope? Any suggestions/improvements you consider related are welcome!

I will write this List problem down and if in future we decide to make the "disabled" public for the ListItem we will have to tackle it. For now, it is fine. Thanks for the feature.

@ilhan007 ilhan007 merged commit e903164 into SAP:master Feb 1, 2021
alexandar-mitsev pushed a commit to alexandar-mitsev/ui5-webcomponents that referenced this pull request Feb 1, 2021
Allow the application specify select options as disabled. Disabled options are non-interactive.

Fixes: SAP#2559
@kineticjs
Copy link
Contributor Author

Thanks!

NHristov-sap pushed a commit to NHristov-sap/ui5-webcomponents that referenced this pull request Feb 9, 2021
Allow the application specify select options as disabled. Disabled options are non-interactive.

Fixes: SAP#2559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui5-select: disabled attribute for option
2 participants