Add job filter for recipes#360
Conversation
This adds a new "Active Job Only" checkbox to the recipe pane which limits the shown recipes/missions to the currently selected job. Equivalent options are also added to the command line, the queries have been converted from nameless tuples to structs, and new tests are added. Implements KonaeAkira#343
augenfrosch
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I was actually planning on working on this before getting sidetracked by my UI code refactor PR, so I have some thoughts on your implementation.
I'm marking this review with "Request changes", since the UI changes have some problems when the available width is limited, e.g., on smartphones, see my comment on recipe_select.rs. The other comments are more nitpicky and are not that important.
It's on the left because putting it on the right while handling the auto-sized field was beyond my egui expertise. Suggestions welcome.
augenfrosch
left a comment
There was a problem hiding this comment.
More general comments this time (including something I missed in my first review, sorry about that).
Looking at the current UI, there are still some things that could be improved:
- There probably should be some visual indication that a filter is currently being used, similar to how buffed stats are highlighted
- The cog
"⚙"looks a bit out-of-place to me, something like a funnel would be more appropriate. But looking at theeguidefault fonts, there doesn't seem to be a glyph for that (but there also isn't a funnel emoji, so that is not really unexpected). Maybe basic shapes could be used to draw a custom funnel icon, whateguidoes for the collapsing header icons came to mind.
But besides the failing CI / Clippy lint (that you already fixed in the time it took me to get around to finishing this review), I think this could be merged as is. I can work on the things mentioned in my comments in a follow-up PR if you prefer, or you could make the changes yourself.
|
If you can work on the two items you listed here in a follow-up, that'd be great. I'm pretty happy with just the cog on the right. |
KonaeAkira
left a comment
There was a problem hiding this comment.
LGTM. Thank you @jmerdich for the PR and thank you @augenfrosch for reviewing ^^
This adds a new "Active Job Only" checkbox to the recipe pane which limits the shown recipes/missions to the currently selected job. Equivalent options are also added to the command line, the queries have been converted from nameless tuples to structs, and new tests are added.
Closes #343