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

UIButton: add bool inputs #4893

Merged
merged 8 commits into from
Aug 27, 2021
Merged

UIButton: add bool inputs #4893

merged 8 commits into from
Aug 27, 2021

Conversation

landism
Copy link
Member

@landism landism commented Aug 25, 2021

Allows buttons to have bool inputs, e.g.

Decided to add this since --dry-run makes for a good example

The TrueString / FalseString fields in UIBoolInputSpec is for convenience to allow direct substitution in stuff like this:

cmd_button('dothing',
           argv=['sh', '-c', './dothing.sh $DRY_RUN'],
           location=location.NAV,
           text='Do a thing',
           inputs=[
             bool_input('DRY_RUN', true_string='--dry-run', false_string=''),
           ]
)

rather than having to do stuff like $(if [[ $DRY_RUN == true ]]; then echo --dry-run; fi) in the command.

Copy link
Contributor

@lizzthabet lizzthabet left a comment

Choose a reason for hiding this comment

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

i tested this locally with the sample button you shared and noticed that the button moves a bit when it's clicked and think you might need to disableRipple (lol) at the dropdown button:
2021-08-26 16 27 34

from a design perspective, i'm not sure it makes sense that the boolean dropdown takes up so much real estate, given that there's only one option ever going to be displayed there. i wonder if making the width smaller would improve it:
Screen Shot 2021-08-26 at 4 29 04 PM
(i'd also be curious about what a toggle control would look like here, instead of a checkbox, because this input is functioning more like an on/off switch, but i don't want to open up a can of design worms! i think it's fine for now.)

web/src/ApiButton.tsx Outdated Show resolved Hide resolved
web/src/ApiButton.tsx Outdated Show resolved Hide resolved
web/src/ApiButton.tsx Show resolved Hide resolved
pkg/webview/view.swagger.json Outdated Show resolved Hide resolved
web/src/ApiButton.tsx Outdated Show resolved Hide resolved
@landism
Copy link
Member Author

landism commented Aug 26, 2021

i tested this locally with the sample button you shared and noticed that the button moves a bit when it's clicked and think you might need to disableRipple (lol)

github doesn't have :lolsob: :(

@landism
Copy link
Member Author

landism commented Aug 27, 2021

the button moves a bit when it's clicked and think you might need to disableRipple (lol) at the dropdown button

  1. good catch!
  2. I've figured out how to disableRipple (apparently ButtonGroup applies its own defaults over top of the Buttons it contains)
  3. the button still moves a bit :(
  4. this happens in master as well, so I filed ch12630 and will tackle that separately

from a design perspective, i'm not sure it makes sense that the boolean dropdown takes up so much real estate, given that there's only one option ever going to be displayed there

It looks like the width in that screenshot is determined by the popup's title, so we'd presumably shrink the font or wrap it?
FWIW, I suspect in the medium term we'll want to replace that dialog with a different component entirely - it doesn't feel great as-is, and was mostly just a thing that already existed and worked well enough.

i'd also be curious about what a toggle control would look like here, instead of a checkbox, because this input is functioning more like an on/off switch

Yeah, that sounds nice. And also maybe give me an excuse to animate our existing toggles :)

@tilt-dev tilt-dev deleted a comment from shortcut-integration bot Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants