Add interactive sort cycling with 's' key and display current sort in footer#331
Closed
benwyrosdick wants to merge 2 commits intoachannarasappa:masterfrom
Closed
Add interactive sort cycling with 's' key and display current sort in footer#331benwyrosdick wants to merge 2 commits intoachannarasappa:masterfrom
benwyrosdick wants to merge 2 commits intoachannarasappa:masterfrom
Conversation
Contributor
benwyrosdick
commented
Oct 17, 2025
benwyrosdick
commented
- Add sort cycling feature with 's' key and display current sort in footer
… footer (#1) * Add sort cycling feature with 's' key and display current sort in footer * Fix code review issues: proper error handling and dynamic width calculation
* Initial plan * Add comma delimiters to dollar amounts in UI display Co-authored-by: benwyrosdick <1733+benwyrosdick@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: benwyrosdick <1733+benwyrosdick@users.noreply.github.com>
Owner
achannarasappa
left a comment
achannarasappa
left a comment
There was a problem hiding this comment.
Overall the sort change looks good.
If you wanted to include the comma related changed, I'd have these high level comments:
- Not necessary to change the name to
ConvertFloatToStringWithCommas - I am thinking it might be valuable to have commas as an option rather than default since there are different conventions in different regions of the world. Open to thoughts on this since a initial solution may be okay without it until there is feedback asking for it.
- Adding commas changes the widths in each column so there should be some testing done on larger size numbers to ensure they don't exceed the max cell width for their respective cells.
| {Text: styleLogo(" ticker "), Width: 8}, | ||
| {Text: styleGroup(" " + groupSelectedName + " "), Width: len(groupSelectedName) + 2, VisibleMinWidth: 95}, | ||
| {Text: styleHelp(" q: exit ↑: scroll up ↓: scroll down ⭾: change group"), Width: 52}, | ||
| {Text: styleHelp(helpText), Width: len(helpText)}, |
Owner
There was a problem hiding this comment.
The overall line width should be less than 80 and handle progressive display otherwise. I'd suggest using VisibleMinWidth to hide parts of this section based on screen width
Owner
There was a problem hiding this comment.
one way is to add just the new sort help text with a VisibleMinWidth which equal to the width of the longest text describing a sort option
Owner
|
I've addressed the previous feedback and merged this change with #340 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.