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

fix: Bring valid selection into multi input component #3213

Merged
merged 8 commits into from
Sep 18, 2020

Conversation

JKMarkowski
Copy link
Contributor

@JKMarkowski JKMarkowski commented Sep 7, 2020

Please provide a link to the associated issue.

part of #3085

Please provide a brief summary of this pull request.

Now multi input uses same structure as list with selection. Temporary there are added some styles, to keep fiori3 design.
Also there is fixed focusing/selecting items by keyboard

Please check whether the PR fulfills the following requirements

@JKMarkowski JKMarkowski added core Core library specific issues 0.22.0 labels Sep 7, 2020
@JKMarkowski JKMarkowski added this to the Sprint 45 - Berlin milestone Sep 7, 2020
@JKMarkowski JKMarkowski requested a review from a team September 7, 2020 11:49
@JKMarkowski JKMarkowski added this to In progress in Development via automation Sep 7, 2020
@JKMarkowski JKMarkowski self-assigned this Sep 7, 2020
@netlify
Copy link

netlify bot commented Sep 7, 2020

Deploy preview for fundamental-ngx ready!

Built with commit ef47910

https://deploy-preview-3213--fundamental-ngx.netlify.app

@JKMarkowski JKMarkowski added blocked blocked ticket and removed blocked blocked ticket labels Sep 9, 2020
Copy link
Member

@mikerodonnell89 mikerodonnell89 left a comment

Choose a reason for hiding this comment

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

Looks good other than this one thing

@@ -68,6 +68,10 @@ export class ListItemComponent implements KeyboardSupportItemInterface, AfterCon
@HostBinding('class.fd-list__item--link')
link = false;

/** Defines if list item is used inside MultiInputComponent */
Copy link
Member

Choose a reason for hiding this comment

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

At the very least this should be hidden from the app developer, but I think we do not need to use an input at all, the multi-input could get list item content children and set the property there. Or this list item could ascend a few parents and figure out if it is in a multi input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mikerodonnell89 You are right, it should be controlled from parent. Anyway it has been changed, so now it uses selection list for multi input with some minor styles adjustments.

Development automation moved this from In progress to Review in progress Sep 9, 2020
Copy link
Member

@mikerodonnell89 mikerodonnell89 left a comment

Choose a reason for hiding this comment

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

I think after opening the popover, user should be able to tab in to the list. Currently I have to click a list item to focus it before I can use the arrow keys to nav through the list

Development automation moved this from Review in progress to Reviewer approved Sep 17, 2020
@JKMarkowski JKMarkowski merged commit 95f0fa3 into master Sep 18, 2020
Development automation moved this from Reviewer approved to Done Sep 18, 2020
@JKMarkowski JKMarkowski deleted the fix/multi-input-selection branch September 18, 2020 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core library specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants