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

Don't show ok button in AddBuildingType if category value is null #5262

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

Helium314
Copy link
Collaborator

Currently when selecting e.g. "for cars" category in AddBuildingType, the ok button is shown, but when clicking ok the user is asked to select a more specific value.
With this change, the ok button is not shown any more when selecting such a category. This should avoid unnecessary attempts on selecting some categories, while at the same time providing a subtle hint that categories can actually be used for answering the quest too.

@mnalis
Copy link
Member

mnalis commented Sep 23, 2023

If I understand correctly, it would only apply to For cars, On a farm and Other categories, while other categories (Residential, Commercial building, Civic building, Religious building) would still retain OK button and be selected? Good idea!

Also, if OK button is not shown in such cases, would it make quest_generic_item_invalid_value text and related code unneeded?

@Helium314
Copy link
Collaborator Author

If I understand correctly, it would only apply to For cars, On a farm and Other categories, while other categories [...] would still retain OK button and be selected?

Exactly

Also, if OK button is not shown in such cases, would it make quest_generic_item_invalid_value text and related code unneeded?

I don't think so. It's been a while, but I think it is/was somehow possible to press the ok button when it shouldn't be visible at all. Possibly related to the appear/disappear animation.
Though maybe the button showing condition is checked again when pressing the ok button, didn't look this up.

@westnordost westnordost merged commit 65b98d6 into streetcomplete:master Sep 25, 2023
@westnordost westnordost deleted the Helium314-patch-1 branch September 25, 2023 14:34
@westnordost
Copy link
Member

Well, whether or not that string could be removed now (and the code that makes it show) would be something for a different PR. Thanks @Helium314

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