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

Settings Search #1312

Merged
merged 43 commits into from
Sep 30, 2023
Merged

Settings Search #1312

merged 43 commits into from
Sep 30, 2023

Conversation

luah5
Copy link
Member

@luah5 luah5 commented Jun 12, 2023

Description

This PR adds support for searching settings in CE's settings, it works very similar to the way System Settings searches.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code
  • Code is clean
  • Highlighted text of search
  • Fix grouping issue

Screenshots

trimmed.mov

Extra notes

I've updated the docs for adding a new setting so that it appears in the search results

@Angelk90
Copy link
Contributor

@luah5 :

Registrazione.schermo.2023-06-13.alle.12.55.26.mov
  1. It would be possible for everyone in the same place to group them.
  2. When clicked, on the text it should open the place where it is which happens, but you don't understand where that text is, it would be possible to make it flash or something similar to how android does when you search for something in the search bar settings.

@austincondiff
Copy link
Collaborator

austincondiff commented Jun 13, 2023

image

You'll notice that:

  1. The text is aligned with the parent item's label
  2. The child item itself is selectable
  3. There is only one parent item, in yours there is a parent for every child
  4. The search term is white while the rest of the text has an opacity (maybe .secondaryLabelColor)

@Angelk90
Copy link
Contributor

@luah5 : When selecting an element I don't think @austincondiff wants the whole group to be selected, at least macOS Ventura doesn't do that, it only selects the single element.

Registrazione.schermo.2023-06-13.alle.21.45.14.mov

For point 2) I think it is useful to understand where the property is, then you need to hear what others think ask @austincondiff.

@austincondiff
Copy link
Collaborator

@Angelk90 I'd agree with out that it should probably flash or jump out at you in some way. System settings doesn't do this though so we might save this for another PR. Regardless, my guess is It is selecting all the related items because they all point to the same location, so they are all selected. We need to think of a workaround to this so only the item that is clicked is selected.

@Angelk90
Copy link
Contributor

@luah5 : The thing that the whole group remains selected does not convince me.

Registrazione.schermo.2023-06-15.alle.11.19.52.mov

@luah5 luah5 requested a review from taylorbyks June 17, 2023 15:01
@luah5 luah5 marked this pull request as ready for review June 18, 2023 14:11
Copy link
Member

@Wouter01 Wouter01 left a comment

Choose a reason for hiding this comment

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

Nice work! I noticed some issues:

  • Tab selection state isn't kept
  • I should be able to go through the search results by pressing the up and down arrow keys
  • It is difficult to search for settings because the search keywords don't match the "real" labels of the settings

This is a good base, but I think it needs improvements. I strongly suggest using settings labels instead of variable names as keywords. This is what will need to happen anyway.
Not doing that now is a bit pointless, as the work from this PR would be removed later on. I would also like to avoid complaints from users about search not being accurate / predictable

CodeEdit/Features/Settings/SettingsView.swift Outdated Show resolved Hide resolved
@austincondiff
Copy link
Collaborator

@luah5 When there is no search text input, the menu item color needs to be primary. It looks like it is defaulting to secondary. It should only be secondary when there is text input into the search field.

@Angelk90
Copy link
Contributor

@austincondiff , @luah5 : When the possibility of having CE in multiple languages will be enabled later and there will be all the translations, how will this system work at the moment?

@0xWDG 0xWDG marked this pull request as draft June 19, 2023 19:04
@austincondiff
Copy link
Collaborator

What is the status on this PR? We need to either finish it or close it. There is a lot of good work here so I'd rather push it over the finish line if we can.

@luah5
Copy link
Member Author

luah5 commented Aug 19, 2023

What is the status on this PR? We need to either finish it or close it. There is a lot of good work here so I'd rather push it over the finish line if we can.

Sorry for the recent inactivity, I'll try to get this finished with my limited time at the moment. Then this PR can be merged.

@luah5
Copy link
Member Author

luah5 commented Aug 21, 2023

I would love to have this PR merged, could some people please review this PR?

Copy link
Contributor

@matthijseikelenboom matthijseikelenboom left a comment

Choose a reason for hiding this comment

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

I've only that remark, other then that it looks fine

CodeEdit/Features/Settings/Models/SettingsPage.swift Outdated Show resolved Hide resolved
@0xWDG
Copy link
Collaborator

0xWDG commented Sep 10, 2023

@luah5 can you fix the conflict with CodeEdit.xcodeproj/project.pbxproj?
Then it should be fine for re-review.

@luah5
Copy link
Member Author

luah5 commented Sep 11, 2023

@luah5 can you fix the conflict with CodeEdit.xcodeproj/project.pbxproj? Then it should be fine for re-review.

sure

@luah5
Copy link
Member Author

luah5 commented Sep 11, 2023

just fixed the merge conflicts, if a reviewer has time, please review this PR thanks!

Copy link
Collaborator

@0xWDG 0xWDG left a comment

Choose a reason for hiding this comment

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

I found a few styling 'issues'.

@0xWDG
Copy link
Collaborator

0xWDG commented Sep 11, 2023

@luah5 FYI, shorthand_operator should not trigger if you use Text() + Text().
see realm/SwiftLint#4936, realm/SwiftLint#4953 for more information.

@austincondiff austincondiff merged commit 5122227 into CodeEditApp:main Sep 30, 2023
2 checks passed
@austincondiff
Copy link
Collaborator

In the interest of preserving the work that has been done and not letting this PR become stale, I merged this PR. For those that requested changes to this PR, feel free to open follow-up PR's for these changes.

@thecoolwinter thecoolwinter added the enhancement New feature or request label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞 Search in settings not working
9 participants