Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Change contract Execute bool values & query bool value display #3024

Merged
merged 5 commits into from
Nov 1, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Oct 31, 2016

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M6-ui A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 31, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 31, 2016
@jacogr jacogr changed the title Allow for boolean value selection Change contract Execute bool values to dropdown Oct 31, 2016
@chevdor
Copy link

chevdor commented Oct 31, 2016

I tested this PR and it fixes the reported issue. Although I would prefer radion buttons (one click less), this solution does the trick and removes any ambiguity from the user whether it is false, 'false', 0, 0x0.... Here there is no choice, which is the best.

Now having to ability to set false values makes another issue apparent: #3025

);
});
inputbox = (
<Select
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not a checkbox or toggle? One click less and more semantical.

Copy link

Choose a reason for hiding this comment

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

  • Checkbox is semantically not the best option.
  • Toggle could work but does not allow for undefined value (the current dropdown solution either).
  • Radio buttons are good because you can have 2 states (true / false), make the entry mandatory and define neither or the states as default so the user must make a choice and he can do so in a single click.

This is a detail, the current implementation is functional.

Copy link
Contributor Author

@jacogr jacogr Oct 31, 2016

Choose a reason for hiding this comment

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

Simple - we don't have a proper wrapped component that fits in with the l&f of the system which includes the labels and hints (not applicable here) as it does for the other inputs. As soon as we bring in a toggle, it raises all kinds of other issues where the specific field won't fit with the rest of the generated UI at this point. So for 1.4 it will be the dropdown.

Not' going to add a different component with the required styling and labels a day before release, especially not after the issues we've had with non-standard components such as the tag inputs.

Copy link

Choose a reason for hiding this comment

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

I was kinda expecting such reason. I think that for now, the dropdown is functional and integrates well in the UI.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 86.02% when pulling 0af5ad1 on jg-contract-execute-bool into 8599a11 on master.

@jacogr jacogr changed the title Change contract Execute bool values to dropdown Change contract Execute bool values & query bool value display Oct 31, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Oct 31, 2016

@chevdor Added a fix for your other issue in this PR as well

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 86.028% when pulling 0af5ad1 on jg-contract-execute-bool into 8599a11 on master.

@chevdor
Copy link

chevdor commented Nov 1, 2016

I don´t see the fix for #3025

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 1, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 1, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 374c2b3 on jg-contract-execute-bool into * on master*.

@chevdor
Copy link

chevdor commented Nov 1, 2016

Tested, all good now. Thank you @jacogr!

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 374c2b3 on jg-contract-execute-bool into * on master*.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 1, 2016
@jacogr jacogr merged commit 314eb59 into master Nov 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants