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

Bbq fuel quest (Issue #4832) #5211

Merged
merged 8 commits into from Oct 13, 2023

Conversation

qugebert
Copy link
Contributor

My first attempt of implementing this quest from Issue #4832

@qugebert qugebert mentioned this pull request Aug 23, 2023
5 tasks
qugebert and others added 4 commits August 24, 2023 12:16
…l/AddBBQFuel.kt

Co-authored-by: Flo Edelmann <git@flo-edelmann.de>
Co-authored-by: Flo Edelmann <git@flo-edelmann.de>
…l/AddBBQFuel.kt

Co-authored-by: Flo Edelmann <git@flo-edelmann.de>
Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Code looks flawless and you thought of every little thing (even attribution etc).

The monochrome icon is pretty boring though, if not ugly. It will be some time till this is merged anyway so I'd take care of this then.

One thing I noticed, generally, while looking at the PR, is that it is kind of really involved to create a new quest as one needs to touch so many files.

Looking at what could be changed, I wonder if the whole PinIcons stuff should be dropped and instead always the icons of the fitting iD presets should be used instead.

Also, I wonder if it was more neatly arranged if the model (in this case, BbqFuel) went into the same file as the quest, as it is somewhat related. I mean, generally, in the future, not for this PR.

What do you think?

@westnordost westnordost self-assigned this Sep 27, 2023
@westnordost
Copy link
Member

(needs icon)

@westnordost westnordost removed their assignment Oct 11, 2023
@westnordost westnordost merged commit 8f38aa3 into streetcomplete:master Oct 13, 2023
@qugebert qugebert deleted the BBQ_Fuel-Quest branch October 13, 2023 18:31
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.

None yet

4 participants