-
-
Notifications
You must be signed in to change notification settings - Fork 650
Conventional Commit Popup #2486
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
Conversation
26c9ab2 to
ae06c25
Compare
Hi @tkr-sh. Thank you for your effort and for sharing this.
If you're still interested, would you be so kind and add a few key screenshots (or even a video capture) that describe this change so we can discuss based on this? I guess this might help this patch along. It's unlikely many people will join the discussion if they have to assert whether a 1k patch does any shenanigans on their development machines or spool up a cloud instance for every iteration and I take it that there will be discussions.
Sure. Tho in theory, it could still work and have untrusted code. In any case, this should be reviewed, and it's straightforward enough so that people realize that there are no shady stuff.
I'll do that in a few hours
Thank you!
Tho in theory, it could still work and have untrusted code.
Sure, screenshots won't change the level of trust by much. They're not so much to inspire trust but to provide the input that's needed for a discussion that can move the PR along.
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.
making this default on will be controversial. I remember we did that in the past and it bloats the binary for a lot of people that are not interested in this
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.
Well, it can be true sometimes, but in this specific case I don't think that it's a good argument because
# With the feature $ cargo build --release && du -b target/release/gitui 15038504 target/release/gitui # Without the feature $ cargo build --release && du -b target/release/gitui 15025560 target/release/gitui
(12944B diff ≈ 0.086% of the total binary size, tested at bb78714)
But you could make the point that most people don't want gitmoji, and in this case, removing it as a default feature is understandable.
Added a video and fixed/upgraded some stuff @naseschwarz @extrawurst
Uh oh!
There was an error while loading. Please reload this page.
It changes the following:
I followed the checklist:
make checkwithout errorsAlso, I know that the UI and the UX is different than the one of Fuzzy finder, but I prefer this one personally (I think that the UX is just better tbh), and it's still easy to follow, even for non vim users :) !
Video:
asciicast
Note: the video is kinda glitched because of the emojis, but everything is aligned