Parameters with a list of defaults / Refactoring / community gocui

[jaroslawhartman]

Issue #, if available:

179 - community github.com/jroimartin/gocui - but anyway this does not seem like a fix. So to minimise impact, textfield made much wider (as the spaces seems inserted when text longer than textfield width)

More changes:

• Refactored params to be a slice rather than map - map causes that params are displayed in random order which is veeery annoying. Now params displayed always in sequence defined in the template.

• New feature 👋: Parameters with default value(s)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[RamiAwar]

Hey @jaroslawhartman , I changed our gocui to the community fork, and that fixed things for me.

We still haven't made a release with that fix yet however. Can you try building pet yourself and checking if the issue persists?

I like the idea of slice instead of map for the parameters though. If that were its own PR I would approve directly. It's usually helpful to make a PR that does one and only one thing, this way you can get your changes approved more easily!

[jaroslawhartman]

Hi @RamiAwar

Can you try building pet yourself and checking if [paste] issue persists?

As I tested with github.com/awesome-gocui/gocui - the problem is no longer there:

Please note that I've also expanded width of the view - IMO it gives much better user experience.

[RamiAwar]

I agree, I think a wider view is nice. The thing is, your PR does quite a few things 😅

1. Change parameter storage from map to slice to maintain order
2. Update the view to be much wider
3. Add support for multiple default values (which is an open discussion currently in other issues)
4. 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:

1. 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.

2. 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.

3. For this one check #192, #203. Not convinced that | is the best separator yet.

4. 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).

[jaroslawhartman]

Hi @RamiAwar

I just want to review each separately, a lot easier to merge stuff in fast and respond to comments if they're separate.

Comments:

1. 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.

2. 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

Fair point. I've crated PR #259

3. Not convinced that | is the best separator yet

Agree; see my comment https://github.com/knqyf263/pet/discussions/247#discussioncomment-8349705

4. 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!

[RamiAwar]

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!

[RamiAwar]

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.