Add a toggleable sidebar to view records #103
Conversation
components/ResultsTable.go
Outdated
| switch stateChange.Key { | ||
| case "Editing": | ||
| editing := stateChange.Value.(bool) | ||
| table.SetIsEditing(editing) | ||
| case "Unfocusing": | ||
| App.SetFocus(table) | ||
| App.ForceDraw() | ||
| case "Toggling": | ||
| table.ShowSidebar(false) | ||
| App.ForceDraw() | ||
| } |
There was a problem hiding this comment.
Editing, Unfocusing, and Toggling should be constants
components/ResultsTable.go
Outdated
| if repeatCount <= 0 { | ||
| repeatCount = 1 | ||
| } | ||
| title += "[gray]" + strings.Repeat("-", repeatCount) |
There was a problem hiding this comment.
Shouldn't gray be a constant you define somewhere to provide a consistent theming
components/Sidebar.go
Outdated
| func (sidebar *Sidebar) FocusNextField() { | ||
| newIndex := sidebar.GetCurrentFieldIndex() + 1 | ||
|
|
||
| if newIndex < sidebar.Flex.GetItemCount() { |
There was a problem hiding this comment.
Invert the test to simplify
| if newIndex < sidebar.Flex.GetItemCount() { | |
| if newIndex >= sidebar.Flex.GetItemCount() { | |
| return | |
| } |
components/Sidebar.go
Outdated
| func (sidebar *Sidebar) FocusPreviousField() { | ||
| newIndex := sidebar.GetCurrentFieldIndex() - 1 | ||
|
|
||
| if newIndex >= 0 { |
There was a problem hiding this comment.
Same check for < 0 and return
components/Sidebar.go
Outdated
| if newIndex >= 0 { | ||
| item := sidebar.Fields[newIndex] | ||
|
|
||
| if item != nil { |
There was a problem hiding this comment.
If item == nil, then return
components/Sidebar.go
Outdated
| case commands.UnfocusSidebar: | ||
| sidebar.Publish(models.StateChange{Key: "Unfocusing", Value: nil}) | ||
| case commands.ToggleSidebar: | ||
| sidebar.Publish(models.StateChange{Key: "Toggling", Value: nil}) |
components/Sidebar.go
Outdated
| case commands.GotoEnd: | ||
| sidebar.FocusLastField() | ||
| case commands.Edit: | ||
| sidebar.Publish(models.StateChange{Key: "Editing", Value: true}) |
components/Sidebar.go
Outdated
| case commands.CommitEdit: | ||
| sidebar.SetInputCapture(sidebar.inputCapture) | ||
| sidebar.SetDisabledStyles(item) | ||
| sidebar.Publish(models.StateChange{Key: "Editing", Value: false}) |
components/Sidebar.go
Outdated
| sidebar.SetInputCapture(sidebar.inputCapture) | ||
| sidebar.SetDisabledStyles(item) | ||
| item.SetText(text, true) | ||
| sidebar.Publish(models.StateChange{Key: "Editing", Value: false}) |
components/constants.go
Outdated
| EditorTabName string = "Editor" | ||
| HelpPageName string = "Help" | ||
| SidebarPageName string = "Sidebar" |
There was a problem hiding this comment.
Unless you think your constant are reused externally, they should be unexported.
So this would be better
| EditorTabName string = "Editor" | |
| HelpPageName string = "Help" | |
| SidebarPageName string = "Sidebar" | |
| editorTabName string = "Editor" | |
| helpPageName string = "Help" | |
| sidebarPageName string = "Sidebar" |
also usually constant of the same kind share a common prefix, not suffix. this way you can use the first letter to find them all in your IDE
So this would be what I would use
| EditorTabName string = "Editor" | |
| HelpPageName string = "Help" | |
| SidebarPageName string = "Sidebar" | |
| tabNameEditor string = "Editor" | |
| pageNameHelp string = "Help" | |
| pageNameSidebar string = "Sidebar" |
jorgerojas26
force-pushed
the
results-sidebar
branch
from
a9b9089 to
c05025a
Compare
jorgerojas26
force-pushed
the
results-sidebar
branch
from
c05025a to
9e8fd49
Compare
|
I'm glad you accepted my suggestion 👍 |
jorgerojas26
force-pushed
the
results-sidebar
branch
from
e4f9e60 to
6e00443
Compare
jorgerojas26
force-pushed
the
results-sidebar
branch
from
6e00443 to
6e055df
Compare
Good suggestions. :) I addressed them all. Thanks for taking the time. |
|
Unless I'm wrong, you didn't apply these suggestions |
You are right haha i missed them. |
| hidingFieldIndex := 0 | ||
| fieldCount := sidebar.Flex.GetItemCount() | ||
|
|
||
| for i := 0; i < fieldCount; i++ { |
There was a problem hiding this comment.
This is a candidate for for range loop, I think
| for i := 0; i < fieldCount; i++ { | |
| for i := range fieldCount { |
There was a problem hiding this comment.
I will need to upgrade go version in go.mod from 1.20 to 1.22. I don't know how much of it is a problem.
There was a problem hiding this comment.
You are providing a tool, not a lib.
So upgrading to 1.23 directly should be fine.
You will have to upgrade a lot of thing such as golangci-lint in the CI
While it's a thing to do, there is no need to do it in this PR, just to use for range
So, yes, you could upgrade, but not necessarily now
jorgerojas26
changed the title
The main idea here is to have that toggleable sidebar that shows the table results in a vertical manner, so it is easy to see all the columns.