Skip to content
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

Add Shift+D command to delete numeric namespace bindings #2268

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

placintaalexandru
Copy link
Contributor

@placintaalexandru placintaalexandru commented Nov 5, 2023

Addresses #2104

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@placintaalexandru Thank you for this update Alexandru!!
I am kind of on the fence with this one...
I think it might be best to enhance the ns view to offer a new col aka favorite and leverage a toggle to opt in/out to make up the favs ns list.
That said, since auto favs ns is still in play, that list will keep changing and user would have to manually curate it. Which feels like a dud.

@derailed derailed added the question Further information is requested label Nov 8, 2023
@placintaalexandru
Copy link
Contributor Author

placintaalexandru commented Nov 8, 2023

@placintaalexandru Thank you for this update Alexandru!! I am kind of on the fence with this one... I think it might be best to enhance the ns view to offer a new col aka favorite and leverage a toggle to opt in/out to make up the favs ns list. That said, since auto favs ns is still in play, that list will keep changing and user would have to manually curate it. Which feels like a dud.

I agree @derailed . I can have a look at enhancing the fav ns thing: by default would be this behaviour that would allow you to delete the numeric bindings, but if the user specifies a flag (e.g., --fav) the fav ns will always be hardoced to the list specified by the user

If you agree, you can close this one and I'll work on what I said above if that is fine

@derailed
Copy link
Owner

@placintaalexandru Thank you Alexandru!! I think we might be able to manage this without hopefully introducing a new flag?? Perhaps if the user sets her fav ns then the auto tracking ns could either be disabled is the fav ns list is full or just managed the list if there is still space in it??

@placintaalexandru
Copy link
Contributor Author

placintaalexandru commented Nov 10, 2023

I mean, I could stil want a self managed list of 4 namespaces and those 4 to never change @derailed
This is why I was thinking to a flag :\

@derailed derailed added the needs-tlc Pr needs additional updates label Nov 12, 2023
Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@placintaalexandru Thank you for this update Alexandru!
I like this feature but it feels a bit heavy to me in its current shape??
Here is a thought. What if we displayed a favorite indicator (❤️) next to the namespace name and add an action something like f Toggle Favorite?
Then users could leverage familiar constructs like multi-select, etc... to manage their fav list?

internal/config/ns.go Outdated Show resolved Hide resolved
internal/view/ns.go Outdated Show resolved Hide resolved
internal/view/ns.go Outdated Show resolved Hide resolved
@derailed derailed added question Further information is requested and removed question Further information is requested labels Nov 21, 2023
@placintaalexandru
Copy link
Contributor Author

placintaalexandru commented Nov 21, 2023

@placintaalexandru Thank you for this update Alexandru! I like this feature but it feels a bit heavy to me in its current shape?? Here is a thought. What if we displayed a favorite indicator (❤️) next to the namespace name and add an action something like f Toggle Favorite? Then users could leverage familiar constructs like multi-select, etc... to manage their fav list?

yeah, SGTM. I'll try to work on this one again

@placintaalexandru
Copy link
Contributor Author

placintaalexandru commented Dec 31, 2023

hello @derailed
There is now the possibility to toggle a namespace as favourite and, in case it exists, to toggle it out from favourite. I use behind data.Namespace.AddFavNS and data.Namespace.RmFavNS.
image

I don't understand 2 things:

  1. Is it still required to limit the number of favourite namespaces to 9?
  2. How would the user manage the fav list? I did not understand the multi-select part

@placintaalexandru placintaalexandru marked this pull request as draft December 31, 2023 11:33
@placintaalexandru placintaalexandru marked this pull request as ready for review January 11, 2024 09:07
@derailed
Copy link
Owner

@placintaalexandru Thank you for the updates and sorry for the delay ;(
The list of fav is constrained by the current key binding aka 0-9.
As for the multi-select while on ns view, I was thinking you can hand pick a few ns and toggle favs to mark or unmark them as favorites??

@placintaalexandru
Copy link
Contributor Author

Hello @derailed

The PR does exactly that, but I was not sure if it had to do something more: it only allows the user to add a namespace to favorite

@derailed
Copy link
Owner

@placintaalexandru Thank you! Once the conflict is resolved I think we can merge...

@placintaalexandru placintaalexandru force-pushed the Add-delete-namespace-shorcut branch 2 times, most recently from cb7796b to 1329b1d Compare August 26, 2024 16:39
@placintaalexandru
Copy link
Contributor Author

placintaalexandru commented Aug 26, 2024

@derailed, I can't seem to be able to build neither the binary nor the tests, not even on the latest master commit.

The problem seems to be this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-tlc Pr needs additional updates question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants