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

refactor(ui5-input): suggestions refactoring #9092

Merged
merged 25 commits into from
Jun 25, 2024
Merged

Conversation

MapTo0
Copy link
Member

@MapTo0 MapTo0 commented May 30, 2024

BREAKING CHANGE: remove type, description, icon, iconEnd, image from ui5-suggestion-item and introduce ui5-suggestion-item-custom

ui5-suggestion-item:

  • type property is removed, use ui5-suggestion-item-custom instead.
  • description property is removed, use ui5-suggestion-item-custom instead.
  • icon property is removed, use ui5-suggestion-item-custom instead.
  • iconEnd property is removed, use ui5-suggestion-item-custom instead.
  • image property is removed, use ui5-suggestion-item-custom instead.

ui5-suggestion-group-item:

  • renamed to ui5-suggestion-item-group
  • text is removed, use headerText instead

ui5-suggestion-item-custom:

  • custom suggestion item with open content similar to ui5-li-custom
  • to be used for custom scenarios
  • to highlight custom items use @ui5/webcomponents-base/dist/util/generateHighlightedMarkup.js

All suggestion items are now physical items and can be overstyled.
Grouping now works with via nesting: e.g.

<ui5-input show-suggestions>
  <ui5-suggestion-item-group header-text="Group 1">
    <ui5-suggestion-item text="Group Item 1"></ui5-suggestion-item>
  </ui5-suggestion-item-group>
</ui5-input>

Related to #8461, #7890

- grouping adoption to the new API
- suggestions are now slotted directly
@MapTo0 MapTo0 requested a review from ndeshev May 30, 2024 10:49
@MapTo0 MapTo0 marked this pull request as ready for review June 13, 2024 10:45
Copy link
Member

@ivoplashkov ivoplashkov left a comment

Choose a reason for hiding this comment

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

Input suggestions with grouping sample:

  1. Type "a" in input then delete it with backspace => items disappear.

Custom Suggestions sample:

  1. Popover does not get scrolled when you navigate through items with keyboard.

Input with disabled autocomplete (type-ahead) and Input with disabled autocomplete and preventable suggestion select:

  1. Popover is being empty when you type something.

Input with open suggestions on focusin sample:

  1. There are a lot of differences from the main branch related to the behaviour of the suggestions, please have a look.

Input - showing wrapping sample :
Screenshot 2024-06-17 at 12 35 57

Input with valueState and Dynamic suggestions sample:

  1. Open the popover so that it has a scroll and navigate through the items with ArrowDown => Popover is not being scrolled properly.
  2. When you focus the ValueStateMessage and spam ArrowUp, focus goes back and forth from the VSM to the Input, which is not expected.

@MapTo0
Copy link
Member Author

MapTo0 commented Jun 19, 2024

Input with valueState and Dynamic suggestions sample:

  1. Open the popover so that it has a scroll and navigate through the items with ArrowDown => Popover is not being scrolled properly.
  2. When you focus the ValueStateMessage and spam ArrowUp, focus goes back and forth from the VSM to the Input, which is not expected.

This is like thhat even in the main branch, I would suggest we fix it with another change since this is a minor issue. Lets focus on the implementation. Rest comments are done

@MapTo0 MapTo0 requested review from ivoplashkov and ndeshev June 19, 2024 14:59
@MapTo0 MapTo0 requested a review from ndeshev June 20, 2024 12:16
@MapTo0 MapTo0 changed the title refactor(ui5-input): custom suggestion item implementation refactor(ui5-input): suggestions refactoring Jun 20, 2024
@MapTo0 MapTo0 requested a review from dobrinyonkov June 20, 2024 13:00
Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

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

There are no tests for inputs with custom suggestion items, please write some.

packages/main/src/Input.ts Outdated Show resolved Hide resolved
}

:host([ui5-suggestion-item]) .ui5-li-content {
padding-bottom: .875rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we have a parameter about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It behaves the same as in the main branch. I noticed that there is no difference between compact and cozy styles which looks strange to me. IMHO this is an existing bug and we should fix it with another change following latest specs of the list.

packages/main/test/specs/Input.spec.js Outdated Show resolved Hide resolved
Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

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

Navigating through the custom items an incorrect speech announcement constructed and will be presented to the SR user
Screenshot 2024-06-21 at 11 26 47
please see selectionText. We should either:

  • try to construct an announcement for custom items
  • remove the announcement if custom items are present, but provide a sample how can an application achieve an announcement on navigating (InvisibleMessaging)

Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

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

  • With your change, non of the sample in the test suite actually filter something - they seem to work to filter a group, but not the items themselves (check the first and second sample). Please adjust at least one sample in order to have working examples with filtering.
  • The SuggestionItemCustom is not present in the playground. Can you please check why?
  • There is no sample of input with custom suggestions in the playground. Please provide one.

Copy link
Contributor

@elenastoyanovaa elenastoyanovaa left a comment

Choose a reason for hiding this comment

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

  1. * **Note:** The and are recommended to be used as suggestion items.

Line 517, Input.ts - not only those items, but the custom items as well.

Check the documentation of the slot for other inconsistencies. For example check the part for the InputSuggestions feature in this section.
2. Not a single test for the input with custom suggestion items - at least the general scenarios should be covered - event firing, typeahead, mobile test.
3. Add the main breaking change topic as Related to (the issue for the breaking changes)
4. Add at least a test sample for the multiinput as well. Good to have: tests for custom suggestion items

packages/main/src/SuggestionItemCustom.ts Outdated Show resolved Hide resolved
@MapTo0 MapTo0 requested a review from elenastoyanovaa June 25, 2024 12:11
@MapTo0 MapTo0 dismissed ivoplashkov’s stale review June 25, 2024 13:19

the feedback has been addressed

@MapTo0 MapTo0 merged commit 36c9c8f into main Jun 25, 2024
10 checks passed
@MapTo0 MapTo0 deleted the cust-suggestion-items branch June 25, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants