You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I agree, I think a wider view is nice. The thing is, your PR does quite a few things 😅
Change parameter storage from map to slice to maintain order
Update the view to be much wider
Add support for multiple default values (which is an open discussion currently in other issues)
Add a way to scroll through these multiple default values (which is a bit opinionated)
Don't mean to discourage you, I think all of these are nice changes that are aligned with Pet's direction for sure, so you really nailed it idea-wise! I've never seen 1) or 4) proposed before in other issues 😀
I just want to review each separately, a lot easier to merge stuff in fast and respond to comments if they're separate.
Comments:
I like it. I think we can already merge this part in if it was separate (without default values, etc.). I can do this one myself to help you out.
I think the view width is a bit too large maybe, a little smaller would look better. It's very rare that someone has such a big parameter value I think. UI wise it's easier on the eyes if things are more centered and not extending to the edges, that's why input forms are usually compact.
I think it's nice to be able to change between the values, although I want to question the UX a little. I think users should be able to see the different options (like an embedded list or potentially a dropdown). I think it's weird to be able to press up and down when you're inside an input field and suddenly get new values. Might accidentally erase your input, or not figure this out without reading the docs. I think it should be clear how to use it without having to read the docs (ex. selection list).
I just want to review each separately, a lot easier to merge stuff in fast and respond to comments if they're separate.
Comments:
I like it. I think we can already merge this part in if it was separate (without default values, etc.). I can do this one myself to help you out.
Thanks for this, much appreciated! Note, somehow I missed updating dialog/params_test.go so this would have refactoring as well.
I think the view width is a bit too large maybe, a little smaller would look better. It's very rare that someone has such a big parameter value I think. UI wise it's easier on the eyes if things are more centered and not extending to the edges, that's why input forms are usually compact
Might accidentally erase your input, or not figure this out without reading the docs. I think it should be clear how to use it without having to read the docs (ex. selection list).
A simple Up/Down key keeps the UI compact, however I agree a list / dropdown list will look cooler. But even with a single-line view, the title can be enhanced with a hint +--- <field name> (press Up / Down for options) ---+
Good point on loosing the text on accidental change; an easy way to protect would be to save current view content to the parameters list and only then swap to the next / previous one.
Cheers!
Thanks for responding! Going to close this PR so we use it as a reference.
I think we can make the up/down key work, but a visual list is still better since users would see which option they are about to select! Makes the selection experience smoother.
As for the loss of text on choosing another, I think that if it were a list and the user made the decision to clearly go to another option, then it's on them. This way we keep it simple, and the UX is very clear!
I think anyway this specific selection issue will have to wait a bit until we deal with the multiple default values issue, maybe I'll move this convo to a discussion or issue.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
Copied from a PR with @jaroslawhartman:
I agree, I think a wider view is nice. The thing is, your PR does quite a few things 😅
Don't mean to discourage you, I think all of these are nice changes that are aligned with Pet's direction for sure, so you really nailed it idea-wise! I've never seen 1) or 4) proposed before in other issues 😀
I just want to review each separately, a lot easier to merge stuff in fast and respond to comments if they're separate.
Comments:
I like it. I think we can already merge this part in if it was separate (without default values, etc.). I can do this one myself to help you out.
I think the view width is a bit too large maybe, a little smaller would look better. It's very rare that someone has such a big parameter value I think. UI wise it's easier on the eyes if things are more centered and not extending to the edges, that's why input forms are usually compact.
For this one check [Feature Request] Support multiple default values for params #192, feat: support multiple default values #203. Not convinced that
|
is the best separator yet.I think it's nice to be able to change between the values, although I want to question the UX a little. I think users should be able to see the different options (like an embedded list or potentially a dropdown). I think it's weird to be able to press up and down when you're inside an input field and suddenly get new values. Might accidentally erase your input, or not figure this out without reading the docs. I think it should be clear how to use it without having to read the docs (ex. selection list).
Hi @RamiAwar
Thanks for this, much appreciated! Note, somehow I missed updating
dialog/params_test.go
so this would have refactoring as well.Fair point. I've crated PR #259
Agree; see my comment #247 (comment)
A simple Up/Down key keeps the UI compact, however I agree a list / dropdown list will look cooler. But even with a single-line view, the title can be enhanced with a hint
+--- <field name> (press Up / Down for options) ---+
Good point on loosing the text on accidental change; an easy way to protect would be to save current view content to the parameters list and only then swap to the next / previous one.
Cheers!
Thanks for responding! Going to close this PR so we use it as a reference.
I think we can make the up/down key work, but a visual list is still better since users would see which option they are about to select! Makes the selection experience smoother.
As for the loss of text on choosing another, I think that if it were a list and the user made the decision to clearly go to another option, then it's on them. This way we keep it simple, and the UX is very clear!
I think anyway this specific selection issue will have to wait a bit until we deal with the multiple default values issue, maybe I'll move this convo to a discussion or issue.
Beta Was this translation helpful? Give feedback.
All reactions