-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Start showing a list of PRs #25
Conversation
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.
Amazing stuff, @heathhenley!
Great idea to copy-paste the implementation with minimal DRY. That's actually was my plan as well: to copy-paste issues and abstract later. Things are in flux still, so it makes sense to keep them decoupled.
There're some similarities between issues and PRs but refactoring always can be done later.
I'm actually working on prettifying issues right now. I'll open a PR with my changes, so it'll be easier to resolve conflicts. You don't need to copy my new changes in this PR, this is also can be done later 🙂
lib/tui/view.ml
Outdated
let pr_section (pr_tab : Model.pull_requests_tab) = | ||
let open Pretty.Doc in | ||
let fmt_state = function | ||
| None -> str "" | ||
| Some Gh.Pr.Merged -> fmt Style.pr_merged "\u{e725}" | ||
| Some Gh.Pr.Open -> fmt Style.pr_open "\u{ea64}" | ||
| Some Gh.Pr.Closed -> fmt Style.pr_closed "\u{ebda}" | ||
in | ||
let fmt_pr (pr : Gh.Pr.t) = | ||
Pretty.Doc.( | ||
horizontal | ||
[ | ||
fmt_state pr.state; | ||
str " "; | ||
fmt Style.secondary (Printf.sprintf "#%d " pr.number); | ||
str pr.title; | ||
str " by "; | ||
fmt Style.bold (Printf.sprintf "@%s" pr.author); | ||
]) | ||
in | ||
pr_tab.pull_requests |> Lazy.force |> List.map fmt_pr |> Pretty.Doc.vertical |
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.
This amazing!
One comment: We're starting to get lots of new view components. Having them inside a single view.ml
file won't scale.
I just quickly implemented issues inside here. But I think a better approach would be to have 3 modules inside the widget/
directory:
code.ml
issue.ml
pr.ml
I'll move code
and issue
but let's start with pr
in this PR (no pun intended).
Just have a single exposed section
function from the Pr
module, so the call site will look like Pr.section
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.
FYI about to force push a rebase with conflicts fixed... |
71427e6
to
ebdad51
Compare
module Pr = Pr | ||
module Generic = Generic | ||
|
||
(** Print the about info into a pretty doc **) | ||
val about_doc : Model.t -> Pretty.Doc.t | ||
|
||
(** Draw the code tab *) | ||
val code_tab : is_selected:bool -> Pretty.Doc.t | ||
|
||
(** Draw the issues tab *) | ||
val issues_tab : is_selected:bool -> Pretty.Doc.t | ||
|
||
(** Draw the pull requests tab *) | ||
val pull_requests_tab : is_selected:bool -> Pretty.Doc.t | ||
|
||
(** Draw the working directory **) | ||
val pwd : string -> Fs.zipper -> Pretty.Doc.t | ||
|
||
(** Draw the file view **) | ||
val file_view : Fs.zipper -> Pretty.Doc.t |
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.
Thanks for creating this file!
It'll make future refactorings easier 🙂
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.
Great stuff!
I really appreciate your work on this project! 🙏🏻
Query for recent PRs and show in a list. Mostly copied the implementation for issues, there's a lot that could be DRY'd out right now as they have basically the same fields - but I think that they might not be so similar after starting to pull more information so I didn't merge them. Not sure if this is anywhere near how you would set it up but let me know what you think, happy to iterate on it.
It looks like this: