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

Fixes #35601 - Search in navigation #9681

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Apr 12, 2023

Screenshot from 2023-05-08 19-07-09

@theforeman-bot
Copy link
Member

Issues: #30344

@theforeman-bot theforeman-bot added Waiting on contributor Legacy JS PRs making changes in the legacy Javascript stack and removed Waiting on contributor labels Apr 27, 2023
@MariaAga MariaAga force-pushed the search-nav branch 2 times, most recently from a18ee38 to 393f9bf Compare June 29, 2023 17:44
@MariaAga MariaAga marked this pull request as ready for review June 29, 2023 17:45
@MariaAga MariaAga changed the title DRAFT - search navigation Fixes #30344 - use pf4 navigation Jun 29, 2023
@MariaAga MariaAga changed the title Fixes #30344 - use pf4 navigation Fixes #35601 - Search in navigation Jun 29, 2023
@MariSvirik
Copy link

MariSvirik commented Jun 29, 2023

@MariaAga could you pls add also a screenshot of an inactive search bar (default state)?

  1. When you make the search bar active by clicking on it (but not typing anything) - is it going to give you all the suggestions? Or would it wait for any letters?

  2. Do you have a screenshot of an empty state?

  3. What is the font size?

@MariaAga
Copy link
Member Author

Here is a link to a demo which hopefully answer most questions: https://hmj7ff.csb.app/ (its a bit jumpy when navigating)
Font size for the result item is: font-size 16px, font-weight 400
Font size for the result item path is: font-size 12px, font-weight 400

@MariSvirik
Copy link

1. I've noticed that sometimes words in the gray breadcrumb are split in the middle e.g. Recurring logic or Config management. Can we fix that somehow? I know it probably depends on the size of your screen. But could we have a rule not to split the words in the middle and just move them to the next line?

Screenshot 2023-07-05 at 15 56 26

2. Also, not sure if that's a bug or a feature:
I need to click twice to actually be able to type anything. So there are actually three states of search which I think is unnecessary.

Screenshot 2023-07-05 at 16 03 27 Screenshot 2023-07-05 at 16 03 31 Screenshot 2023-07-05 at 16 03 44

3. What's more, there is an error when I start typing.

Screenshot 2023-07-05 at 16 06 06

@MariaAga
Copy link
Member Author

MariaAga commented Jul 5, 2023

@MariSvirik Thanks!

  1. Fixed in the demo https://hmj7ff.csb.app/host_statuses
  2. I'm using the expendable search like here: https://www.patternfly.org/v4/components/search-input#with-expandable-button
    Which does focus on the text if users click on the search glass, originally the space next to the search glass icon shouldnt be clickable at all.
    Should I just use a normal search instead of an expandable one?
  3. Fixed in the demo

@adamruzicka
Copy link
Contributor

@MariSvirik ping

@MariaAga
Copy link
Member Author

@MariSvirik I also made a demo that doesnt use expendable search, but just a search input here: https://l9hdpm.csb.app/audits
What do you think?

@github-actions github-actions bot removed the Legacy JS PRs making changes in the legacy Javascript stack label Oct 17, 2023
@MariaAga
Copy link
Member Author

Talked to Maria S and we decided on this design:
Screenshot from 2023-10-17 21-45-54
@ofedoren Can you please review this PR?

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @MariaAga! Great addition. No more trying to remember where is $ITEM located :)

Few comments:

  • Is there a way to trigger the nav search via keyboard? Looking at the tickets, there was an experimental key ` as an example.
  • It would be nice if the first item from the suggestion list was auto-selected, so I don't need to use arrows for explicit selection.
  • Some comments inline.
  • Test failures seem to be related?
  • Huge thanks for the explanations in the comments :)

@MariaAga
Copy link
Member Author

MariaAga commented Nov 7, 2023

Thanks!

Is there a way to trigger the nav search via keyboard? Looking at the tickets, there was an experimental key ` as an example.

added CTRL + SHIFT + F shortcut as ` was mentioned not being easily accessible in some keyboard layouts

It would be nice if the first item from the suggestion list was auto-selected, so I don't need to use arrows for explicit selection.

I'm not sure if that what you meant, but now if a user starts typing in the search and clicks ENTER the page will redirect to the first result

Added more specific selectors to failing tests

ofedoren
ofedoren previously approved these changes Nov 7, 2023
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @MariaAga! One inline question, but otherwise good to go :)

const searchInput = (
<SearchInput
value={value}
placeholder={__('Search and go')}
Copy link
Member

Choose a reason for hiding this comment

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

Could we change it to Ctrl + Shift + F to search? I mean, how would one know what to use?

Not a big deal though, since that combination is used in a lot of editors...

Copy link
Member Author

@MariaAga MariaAga Nov 9, 2023

Choose a reason for hiding this comment

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

I talked to Maria from UX about this, and since we (and other projects) dont show the / shortcut in the search bar placeholder we shouldn't do it here as well. I'll get it in the docs and I added it to the about page
image

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Love it, thanks @MariaAga! Not sure though if we can merge it now or should we wait for branching...

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @MariaAga, apparently with this one we can go further :) Promise me, you'll re-check we didn't break anything for most wanted plugins :D

@ofedoren ofedoren merged commit 8b1890e into theforeman:develop Nov 15, 2023
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants