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

Add search fields to the map editor template and actor lists. #13596

Merged
merged 3 commits into from Jul 16, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Jul 5, 2017

This PR wraps up @rob-v's editor improvements by adding a search field to filter actors/tiles by name/id/category/tooltip. It also adds the template ID to the current cell display so that mappers can quickly identify and filter/select any tile on the map.

screen shot 2017-07-05 at 18 56 30

screen shot 2017-07-05 at 13 49 02

Supersedes #13337.

Depends on #13339, #13353.

@pchote pchote added this to the Playtest featuring updated HitShapes milestone Jul 5, 2017

@pchote pchote requested a review from rob-v Jul 5, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 5, 2017

Contributor

Dependencies have been merged, needs rebase.

Contributor

reaperrr commented Jul 5, 2017

Dependencies have been merged, needs rebase.

@pchote pchote removed the PR: Rebase me! label Jul 6, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 6, 2017

Member

Done.

Member

pchote commented Jul 6, 2017

Done.

@rob-v

It is great! I am glad I closed my PR as this is improved and with additional optimizations.

Only small issues +
I am not sure about the inclusion of categories in search terms. E.g. I search for 'Civilian'. Usually you see actors where search term is in the name or tooltip, but if there is also category with such name, there are/could be many other actors without search term in name so you need to search manually.
It might be better to remove categories from search term as they can be already filtered out by selecting categories in the list - my feeling was that it is little confusing as the results aren't consistent (that search term is in tooltip).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 6, 2017

Member

Fixed.

Member

pchote commented Jul 6, 2017

Fixed.

@rob-v

One issue.

note(issue): you start with e.g. None. search for 1tnk. There are 2 filtered categories. When you use 'All', not only these 2 categories are selected, but when you delete search text, all categories are selected. Maybe not better/simple to select with All/None only visible/fitlered categories?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 8, 2017

Member

you start with e.g. None. search for 1tnk. There are 2 filtered categories. When you use 'All', not only these 2 categories are selected, but when you delete search text, all categories are selected. Maybe not better/simple to select with All/None only visible/fitlered categories?

IMO this behaviour is more logical than having the "all" button hide categories after returning if you press it while searching.

Member

pchote commented Jul 8, 2017

you start with e.g. None. search for 1tnk. There are 2 filtered categories. When you use 'All', not only these 2 categories are selected, but when you delete search text, all categories are selected. Maybe not better/simple to select with All/None only visible/fitlered categories?

IMO this behaviour is more logical than having the "all" button hide categories after returning if you press it while searching.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jul 8, 2017

Contributor

Why should you hide categories? In your example:
image
You can manually check those 2 categories or with 'All' button at once only these 3 visible. You check what is visible, no need to hide something lately and nothing is checked in background.

Contributor

rob-v commented Jul 8, 2017

Why should you hide categories? In your example:
image
You can manually check those 2 categories or with 'All' button at once only these 3 visible. You check what is visible, no need to hide something lately and nothing is checked in background.

@reaperrr

Looks and works good 👍

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 9, 2017

Contributor

Actually, (assuming we mean the same thing), I agree with @rob-v. If I had selected 'None' before I start typing into the search field, I'd expect the editor to automatically switch to 'Search Results' as soon as I type in anything. Having to manually switch from None to All or select one of the listed categories to see what the search found is not intuitive, in my opinion.

Contributor

reaperrr commented Jul 9, 2017

Actually, (assuming we mean the same thing), I agree with @rob-v. If I had selected 'None' before I start typing into the search field, I'd expect the editor to automatically switch to 'Search Results' as soon as I type in anything. Having to manually switch from None to All or select one of the listed categories to see what the search found is not intuitive, in my opinion.

@rob-v

rob-v approved these changes Jul 9, 2017

👍
As mentioned, I think it would be better if All and None worked only on filtered/visible categories, anyway it works fine.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jul 9, 2017

Contributor

@reaperrr Your issue and my issue are different I guess.

Contributor

rob-v commented Jul 9, 2017

@reaperrr Your issue and my issue are different I guess.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 9, 2017

Contributor

Well, the fixup label is only about the IsTraitEnabled, apart from that I think this can be merged as-is.

Contributor

reaperrr commented Jul 9, 2017

Well, the fixup label is only about the IsTraitEnabled, apart from that I think this can be merged as-is.

@reaperrr reaperrr merged commit 4ffcd9b into OpenRA:bleed Jul 16, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pchote pchote deleted the pchote:map-search branch Oct 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment