Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Issue #13: Sort item list by when to buy urgency #35
Issue #13: Sort item list by when to buy urgency #35
Changes from all commits
badabe5
6d8fdd0
cece0fb
9c0bf6f
091c3c7
42dea27
694d7f8
41e2ef2
b2e3fa9
6951bfd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this! Did you consider an enum here? There's pros and cons:
Priority.Low
,Priority.Medium
,Priority.High
. Now you don't need to write down what-1
means. To some, that might be misunderstood to be very low priority. Careful documentation can get around that. You can also assign enums values (see some of the examples here) so you can still keep the numbers where you need them (if we're putting this in a database, for instance) but give them "more meaning" in the code.tldr; maybe consider enums with values assigned to them 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 I just realized this is for a
sort
- forget everything I said! Maybe we could leave a comment above this function explaining what it's used for and a brief overview of what it considers a "high priority" vs "low priority" ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRAISE: I like the use of .sort() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! Great detail to implement the sorting even within the filtered list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta pick your guys' brain a bit tomorrow about the logic of this piece of code :) I was trying to wrap my head around if
comparePurchaseUrgency
is only being called in the the action of filtering the search term, but of course that's not the case because the UI is being returned in the unfilteredList..update:
Ahhh. I see that the filteredListItems is just checking if the searchTerm is included in the list then sorting the items based on urgency. I thought perhaps only items in the filtering of items by name, were being sorted.