feat: showing a loading message #364

kyu08 · 2024-05-19T09:04:06Z

Resolves #363

This is my first PR to gh-dash. So if I'm doing something wrong. Please let me know that. 😅

Summary

Show Loading... when gh-dash fetching PRs(not issues because the Tip is showed when issues are being fetched currently)

How did you test this change?

Verify Loading... is showed when:

gh-dash is launched
r key is pushed
R key is pushed

Images/Videos

Before

before.mov

After

after.mov

I considered to showing Loading... on sidebar too. But the message currently showed is suitable too. So I did not change it.

kyu08 · 2024-05-19T09:06:20Z

ui/components/issuessection/issuessection.go

@@ -119,6 +119,7 @@ func (m Model) Update(msg tea.Msg) (section.Section, tea.Cmd) {
 					currIssue.Assignees.Nodes = removeAssignees(currIssue.Assignees.Nodes, msg.RemovedAssignees.Nodes)
 				}
 				m.Issues[i] = currIssue
+				m.Table.SetIsLoading(false)


If L122 and L136 is not added, issues can not be shown because Loading... is showed always.

dlvhdr · 2024-05-24T16:11:18Z

ui/components/table/table.go

+	if m.isLoading {
+		return bodyStyle.Render(m.loadingMessage)
+	}


If you're up for it, let's make a bit more fancy.
You can add a padding of 1 to the left (I think) and it should center it better.
Also, you can add a spinner to it's left with "github.com/charmbracelet/bubbles/spinner". You can see an example in https://github.com/charmbracelet/bubbletea/tree/master/examples/spinner

Thanks for taking time for this PR!
Wow, this spinner is great! I'll try to do it!

@dlvhdr

I tried in c2f2405.
But the spinner does not work.
I will try to fix later.
If you have an idea please let me know.

Untitled.mov

I tried in c2f2405.
But the spinner does not work.
I will try to fix later.
If you have an idea please let me know.

@dlvhdr
This problem has been fixed.
I added loadingSpinner, loadingSpinnerCmd := m.Table.LoadingSpinner.Update(msg) only to issuesection.go.
I have to add it to prsection.go as well. 😅

After I will been done with TODOs and review the code sanity, I will ask you to review.

@dlvhdr
I'm done with fixing.
Could you review this PR please?

dlvhdr · 2024-05-30T20:14:25Z

Thanks for working on this @kyu08, looks great!
Just a couple of comments:

When I scroll to the end of a list, the behavior currently, is to fetch the next page of PRs/Issues. Right now it looks odd that the whole screen replaces with a loading message. I believe you can check whether we are fetching the first page or whether it's a pagination action. To see this, just have a query return more than 20 PRs and scroll to the bottom with G
The color of the loading indicator should use the colors from the user's theme. I think ctx.Theme.SecondaryText could work well. WDYT?

Omnikron13 · 2024-05-31T19:43:39Z

I don't actually recall what it does atm with a long list... It paginates?

Wouldn't be it beter to behave like, say, vim and not actually highlight the bottom couple until it's actually the bottom couple? That's how most people configure their text editor, and I believe this behaviour is supported by native components isn't it?

dlvhdr · 2024-05-31T20:39:56Z

wdym @Omnikron13? Like vim's scrolloff?

Omnikron13 · 2024-05-31T20:48:10Z

Yeah. At the resolution I'm normally at it takes a hell of a large list to actually see what happens with a full screen+ of items. Bumping up my font size to comical levels it seems that is the current behaviour? Is that configurable? I haven't dug too much into config options honestly

…

On 31 May 2024 21:40:17 BST, Dolev Hadar ***@***.***> wrote: wdym @Omnikron13? Like vim's `scrolloff`? -- Reply to this email directly or view it on GitHub: #364 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

Omnikron13 · 2024-05-31T20:56:25Z

My lax attitude to issues and PRs is rather why good tooling that doesn't make me leave the terminal to pleasantly interact with... well, practically every part of GitHub tbh. Spent literal a couple years using a daily driver that didn't even have X installed out a general preferences, and by god have things god notably more pleasant since then as well.

kyu08 · 2024-06-01T10:33:39Z

@dlvhdr
The color issue has been fixed in 58e9fca.
The pagination issue has been fixed in b6d5806.

Could you review these changes please?

dlvhdr · 2024-06-01T16:29:42Z

Thanks for fixing my comments :)
I think there's another small bug, let me know if you want to fix it or land as is.
When refreshing every view with R (capital R) the spinner doesn't spin.

kyu08 · 2024-06-02T03:23:42Z

@dlvhdr
Thanks for reviewing too!

I think there's another small bug, let me know if you want to fix it or land as is.
When refreshing every view with R (capital R) the spinner doesn't spin.

In my environment, I could not confirm reproduction.
Can you show me reproduction procedure please?

dlvhdr · 2024-06-02T07:00:00Z

Not near a computer right now but when you press R you can see the spinner properly working but then when you switch to the next section with l it stops.

kyu08 · 2024-06-02T09:47:58Z

@dlvhdr

Thank you for letting me know how to repro.
Now I can repro on my environment too.

I took a look around the codes, but I couldn't identify the cause.

For now, how about we merge this PR and discuss this issue separately as you suggested?

dlvhdr · 2024-06-02T13:46:07Z

Opened #386

kyu08 · 2024-06-02T13:47:31Z

Thanks!
When I have enough time, I'll take a look at it.

feat: showing a loading message

06cfb6b

kyu08 commented May 19, 2024

View reviewed changes

dlvhdr reviewed May 24, 2024

View reviewed changes

add spinner

5cd426a

kyu08 force-pushed the feat-loading-state branch from c2f2405 to 5cd426a Compare May 26, 2024 12:11

kyu08 added 2 commits May 26, 2024 21:30

centerize loading message

3cf90f6

add Table#Update

f325110

kyu08 requested a review from dlvhdr May 26, 2024 13:50

use the colors from the user's theme

58e9fca

hide loading message during pagination

b6d5806

dlvhdr merged commit 9d27687 into dlvhdr:main Jun 2, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: showing a loading message #364

feat: showing a loading message #364

kyu08 commented May 19, 2024 •

edited

Loading

kyu08 May 19, 2024

dlvhdr May 24, 2024

kyu08 May 24, 2024

kyu08 May 25, 2024 •

edited

Loading

kyu08 May 26, 2024 •

edited

Loading

kyu08 May 26, 2024

dlvhdr commented May 30, 2024 •

edited

Loading

Omnikron13 commented May 31, 2024

dlvhdr commented May 31, 2024

Omnikron13 commented May 31, 2024 via email

Omnikron13 commented May 31, 2024

kyu08 commented Jun 1, 2024

dlvhdr commented Jun 1, 2024

kyu08 commented Jun 2, 2024

dlvhdr commented Jun 2, 2024

kyu08 commented Jun 2, 2024

dlvhdr commented Jun 2, 2024

kyu08 commented Jun 2, 2024

feat: showing a loading message #364

feat: showing a loading message #364

Conversation

kyu08 commented May 19, 2024 • edited Loading

Summary

How did you test this change?

Images/Videos

Before

After

kyu08 May 19, 2024

Choose a reason for hiding this comment

dlvhdr May 24, 2024

Choose a reason for hiding this comment

kyu08 May 24, 2024

Choose a reason for hiding this comment

kyu08 May 25, 2024 • edited Loading

Choose a reason for hiding this comment

kyu08 May 26, 2024 • edited Loading

Choose a reason for hiding this comment

kyu08 May 26, 2024

Choose a reason for hiding this comment

dlvhdr commented May 30, 2024 • edited Loading

Omnikron13 commented May 31, 2024

dlvhdr commented May 31, 2024

Omnikron13 commented May 31, 2024 via email

Omnikron13 commented May 31, 2024

kyu08 commented Jun 1, 2024

dlvhdr commented Jun 1, 2024

kyu08 commented Jun 2, 2024

dlvhdr commented Jun 2, 2024

kyu08 commented Jun 2, 2024

dlvhdr commented Jun 2, 2024

kyu08 commented Jun 2, 2024

kyu08 commented May 19, 2024 •

edited

Loading

kyu08 May 25, 2024 •

edited

Loading

kyu08 May 26, 2024 •

edited

Loading

dlvhdr commented May 30, 2024 •

edited

Loading