Skip to content

feat(ui5-shellbar-search): initial implementation #11398

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MapTo0
Copy link
Member

@MapTo0 MapTo0 commented Apr 23, 2025

No description provided.

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 is not a single test that check the typeahead on mobile - it is not covered in the Search as there the typeahead comes from the input. Please write a basic coverage of this functionality

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. Should we add a sample to the playground? Maybe in the Shellbar, or for sure - add the documentation of the component to the playground.

  2. The spec of the Shellbar contains to following acc:
    Screenshot 2025-04-24 at 13 36 45
    Check with team Pirin if we should do it by default in the ShellBarSearch, or the APP should be able to change it. If the second option is decided (or generally I think this is a good idea) - ariaDescription/ariaDescriptionRef should be enabled on SeachField level as a property.

  3. Where can I test the new component - there is no test page? At least enrich the ShellBar_evolution.html page.

@MapTo0 MapTo0 requested a review from elenastoyanovaa April 25, 2025 12:19
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.

I approve the changes, but @dobrinyonkov @MapTo0 please provide a working example with a shellbar search, as the current does not work on mobile and if someone wants to test he has to build his own sample. Also a playground sample should be provided as well.

@@ -48,7 +48,7 @@ describe("SearchField general interaction", () => {
cy.get("[ui5-search-field]")
.shadow()
.find("input")
.should("have.attr", "aria-description", SEARCH_FIELD_LABEL.defaultText);
.should("have.attr", "aria-label", SEARCH_FIELD_LABEL.defaultText);
Copy link
Contributor

Choose a reason for hiding this comment

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

The test is for accessibleDescription, please adjust the naming at least.

@dobrinyonkov
Copy link
Contributor

dobrinyonkov commented Apr 28, 2025

All shellbar's samples will be UXC ajdusted. After this gets merged, the new shellbar search with replace the input with #11340.

In addition, initially opened full-width search field was considered inconvenience and will be prevented by #11419.

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.

3 participants