Add a 'Sellable' list #16
Conversation
|
As mentioned here, this excludes any item that has no quest or upgrade purpose for the time being. |
|
Also, known 'bug': the check for items completed compares to the omitted list for now, so may consider all items sellable if one has completed all mission requirements and omits upgrades or vice versa. |
|
Appreciate this, want to inspect the code a little more closely since this is a sizeable change but will merge or suggest changes time permitting. |
| {focusQuestsCount > 0 ? focusedQuests : null} | ||
| {itemList} | ||
| {focusQuestsCount === 0 && omittedItems === null ? sellableList : null} | ||
| </div>)} |
There was a problem hiding this comment.
I can't decide if I like this way of organizing elements. Part of me feels like if you're going to separate all of them, you may as well make them components. Mind convincing me this is the way to go? I agree it's generally more readable than just having all the markup loose, but, what are your thoughts on taking the elements here and making them components versus doing it this way?
There was a problem hiding this comment.
No argument whatsoever, really. This is my first foray into React; I mostly do backend Go/Python so if anything it was to help myself understand the current structure and learn how to refactor etc. In that spirit, I'm probably in favour of whatever allows maintainability and extensibility. I can see why that might be more granular components, if appropriate.
There was a problem hiding this comment.
Gotcha. Well I'm impressed, and I was actually going to reach out and ask if you were looking for a job at all, I need to hire a senior FE dev, I love your attention to detail already.
About the components, I know some people will do this kind of thing, I'm not used to it, but that doesn't mean it's wrong. The beauty/ugly of react is you can kinda do whatever and none of it is "wrong". I'm okay with leaving it as-is but if these elements were reused later I think I'd rather see them as components. I might prefer to see them as components anyways just so this file doesn't get too big, as it kind of is, but, having a ton of files with a few lines isn't always helpful either. We can just run with it and switch it up later if it's a problem. I think this is actually a pretty sensible way to organize these elements within a component and I'm just experiencing an existential struggle as I challenge my worldview :)
There was a problem hiding this comment.
It's always nice to know I'm not the only one learning something through a PR! The apparent structural flexibility of React (JS/TS, really) is quite a shift from what I'm used to, so knowing the pros and cons of a situation is super helpful, thank you.
If the current state is or gets too unwieldy, refactoring into components sounds like another learning opportunity, so raise an issue if so and I'll give it a spin.
As for the offer, I'm very flattered but in a good spot at the moment I think. You never know what's around the corner though 🤷
No rush, I have my local instance running so am now getting the benefit anyway. I'm new to JS/TS so still a good learning experience. |
|
Happy with this from a code perspective, will merge when I can get home and look at it locally. I might tweak the UX a little depending on how it feels to me, but, looks good and thank you for your contribution! 👍 |
|
I think I want to tweak this so it's more compatible with the quest focusing feature, I'll use this as an excuse to add the settings page and then add some finer controls for what/when this list shows up. |
Also a reasonable refactor of the
ItemListcomponent, for readability regarding conditionals etc