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: add YaruSearchField and YaruSearchTitleField #734

Merged
merged 21 commits into from
Jul 27, 2023
Merged

Conversation

Feichtmeier
Copy link
Member

@Feichtmeier Feichtmeier commented Jul 20, 2023

I thought it is time to find a sane default for searchfields and searchfields inside titlebars since this seems to become a common pattern in our apps.

This does not need to be the final result and should eventually be parameterized more but this could be a good start.

And in general with flutter, aynthing is changeable.

Opinions @madsrh @Jupi007 @jpnurmi @elioqoshi ?

SearchActive SearchNotActive
Bildschirmfoto 2023-07-20 um 14 14 11 Bildschirmfoto 2023-07-20 um 14 14 23
Bildschirmfoto 2023-07-20 um 14 14 45 Bildschirmfoto 2023-07-20 um 14 15 01

@jpnurmi
Copy link
Member

jpnurmi commented Jul 20, 2023

Did it somehow stretch the title bar or does it just look extra tall because of the macOS window controls?

@Feichtmeier
Copy link
Member Author

Feichtmeier commented Jul 20, 2023

I think I just hit CMD+ + and then in vscode even the flutter window increases its titlebar :)

this is how it looks on windows
grafik
grafik

the way the example is built I can't just put a scaffold as a page. its possible but I would need to edit aaaaaall page item pages which is kind of overkill for this PR

@Jupi007
Copy link
Member

Jupi007 commented Jul 20, 2023

I really like it!

@madsrh
Copy link
Member

madsrh commented Jul 21, 2023

I thought it is time to find a sane default for searchfields and searchfields inside titlebars since this seems to become a common pattern in our apps.

💪 Sounds like a good idea

Looks good but TBH I'm not a fan of the two background colors. Could you explain again why you want the icon to have a different background color? Is it clickable? I'm sorry this was mentioned elsewhere

@Feichtmeier
Copy link
Member Author

yes its an icon button. when you click it the searchbar is layed under it (stack)

@madsrh
Copy link
Member

madsrh commented Jul 21, 2023

yes its an icon button. when you click it the searchbar is layed under it (stack)

Makes sense. I couldn't tell from the screenshot 😅

@Feichtmeier Feichtmeier marked this pull request as draft July 21, 2023 10:57
@Feichtmeier
Copy link
Member Author

need to improve docs and more parameters and make te search button available without the title so you can either have

all-in-one widget YaruSearchFieldTitle
or
YaruSearchField in the title plus YaruSearchButton elsewhere

also I want to finish the yaru size PR first then I can remove the constants here

@Feichtmeier
Copy link
Member Author

Bildschirmaufzeichnung.vom.2023-07-24.17-38-14.webm

ok I think this is pretty good
yet modular enough I hope

if there is an onClear callback provided the suffix icon shows up with a clear button

the search field and button can be used either combiend in the title or separated

@Feichtmeier Feichtmeier marked this pull request as ready for review July 24, 2023 15:59
@Feichtmeier Feichtmeier requested a review from jpnurmi July 24, 2023 15:59
@jpnurmi
Copy link
Member

jpnurmi commented Jul 24, 2023

I understand well why, but this looks mis-aligned when the search is not active. :)

image

@Feichtmeier
Copy link
Member Author

yeah the example is stupid here because you would prbly not change the title in the text field 😸
I'll see if I can improve or parameterize the space between the title and the search icon

lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Show resolved Hide resolved
lib/constants.dart Outdated Show resolved Hide resolved
@Feichtmeier
Copy link
Member Author

@jpnurmi thanks for the review will check later and fix

example is now a bit clearer and I added an alignment parameter for the title widget

grafik

@Feichtmeier
Copy link
Member Author

Thanks @jpnurmi there were indeed some heavy issues 🙈

@Feichtmeier Feichtmeier requested a review from jpnurmi July 24, 2023 18:25
@Feichtmeier Feichtmeier marked this pull request as draft July 24, 2023 18:42
@Feichtmeier Feichtmeier removed the request for review from jpnurmi July 24, 2023 18:58
@jpnurmi
Copy link
Member

jpnurmi commented Jul 25, 2023

A couple of remarks, feel free to ignore them if they are not relevant.

  • There's something that makes it feel somehow... unlike Yaru. Perhaps the fully rounded corners? It's not necessarily bad, but no other search field looks like this on my desktop. :)
  • The name "search field title" feels somehow backward. I'm not sure if "search title field" would sound any more natural but it would perhaps emphasize better that it's still a field, not just the title of a field.

@Feichtmeier
Copy link
Member Author

Well, I could just add a border radius parameter and you guys choose what you like.
Personally I feel that those widgets need an iconic look because they fulfill a special purpose. They're not regular Text fields like in forms. (Not saying forms are not important)

I'll change the name! 😎👌

@Feichtmeier Feichtmeier marked this pull request as ready for review July 25, 2023 19:06
@Feichtmeier
Copy link
Member Author

@jpnurmi renamed as requested plus added a radius parameter
you might ask 🤔 "why not BorderRadius?"
because the suffix icon has only the right edges rounded :)

image
example with kYaruButtonRadius from yaru.dart

@jpnurmi jpnurmi changed the title feat: add YaruSearchField and YaruSearchFieldTitle feat: add YaruSearchField and YaruSearchTitleField Jul 26, 2023
@jpnurmi
Copy link
Member

jpnurmi commented Jul 26, 2023

The search icon button is 35x35px whereas the clear icon button is 40x35px:

image

The clear button can be clicked even if the search field is empty. It's not immediately obvious that you need to click the search button again to close the search field. Perhaps the clear button could close the field if it's already empty?

Screencast.from.2023-07-26.08-18-49.webm

@Feichtmeier
Copy link
Member Author

The search icon button is 35x35px whereas the clear icon button is 40x35px:

image

The clear button can be clicked even if the search field is empty. It's not immediately obvious that you need to click the search button again to close the search field. Perhaps the clear button could close the field if it's already empty?
Screencast.from.2023-07-26.08-18-49.webm

I would leave the logic when to enable the clear button via the callback to the user of the widget

I'll fix the clear button width

@Feichtmeier
Copy link
Member Author

Feichtmeier commented Jul 27, 2023

@jpnurmi nurmi

fixed width of the clear button

plus added a YaruSearchFieldStyle
filled, outlined, filledOutlined parameters
plus filledcolor and bordercolor (optional)

filled
grafik

outlined
grafik

filledoutlined
grafik

if you combined this the radius property you receive your desired heavy look
grafik

anything else?

Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

I still think the clear button should close an empty search field or at least become disabled if it doesn't have any effect. Also, the search field is left open forever even when it's empty and loses focus. It's not immediately obvious how to get rid of the field and return back to the normal title. Especially since the clear button is confusingly enabled yet does nothing... :) Should either YaruSearch(Title)Field or the example handle these cases?

lib/src/widgets/yaru_search_field.dart Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Outdated Show resolved Hide resolved
lib/src/widgets/yaru_search_field.dart Show resolved Hide resolved
Feichtmeier and others added 5 commits July 27, 2023 13:04
Co-authored-by: J-P Nurmi <jp.nurmi@canonical.com>
Co-authored-by: J-P Nurmi <jp.nurmi@canonical.com>
Co-authored-by: J-P Nurmi <jp.nurmi@canonical.com>
Co-authored-by: J-P Nurmi <jp.nurmi@canonical.com>
@Feichtmeier
Copy link
Member Author

@jpnurmi can I merge?

Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Feichtmeier Feichtmeier merged commit 0cc96d8 into main Jul 27, 2023
7 checks passed
@Feichtmeier Feichtmeier deleted the search_field branch July 27, 2023 11:33
@github-actions github-actions bot mentioned this pull request Jul 27, 2023
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.

4 participants