Made it so that nullable radio defaults correctly (fix #545) #589

zacbraddy · 2018-09-30T23:06:43Z

Description

Made it so that when you focus the text input for a text field that is nullable then if the value in the text field is undefined, null, or an empty string then the null radio box for that field with be checked.

Also whilst typing in the text input of a nullable text field if you add characters, thus making the value not an empty string, then the radio to the left of the field is checked and the null radio is unchecked. If you then remove all characters leaving an empty string the radio to the left is unchecked and the null radio is checked.

The radio buttons themselves can be changed after having entered or removed a value in the text input but the behaviour above allows for the UI to choose sensible defaults for the user.

What component does this PR affect?

Requires changes from other components? If yes, please mark the components:

Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Docs update
Community content

Checklist:

I have read the contributing guide and my code conforms to the guidelines.
This change requires a change in the documentation.
I have updated the documentation accordingly.
I have added required tests.

… enter something it selects the left radio button.

CLAassistant · 2018-09-30T23:06:50Z

All committers have signed the CLA.

hasura-bot · 2018-09-30T23:27:47Z

Review app available at: https://hge-ci-pull-589.herokuapp.com

karthikvt26 · 2018-10-01T11:46:13Z

LGTM. @praveenweb can you please take a look?

praveenweb · 2018-10-02T05:38:06Z

@zacbraddy - Hey, just verified your implementation in the above sample heroku app. But doesn't seem like it has been fixed. When i try to type in a text field which is nullable, the left radio has to be selected (typical onChange handler). But it doesn't get selected as expected. Can you recheck?

praveenweb · 2018-10-02T05:55:04Z

@zacbraddy - Just to clarify, your implementation works only for text fields. The expectation is that it should work for all types of fields, like integer, timestamp and boolean fields etc. Let me know if you want to proceed working for that implementation :)

zacbraddy · 2018-10-02T10:26:47Z

Hi there @praveenweb

Yep, just a misunderstanding on my part mate. I thought this was only something you wanted done on text fields. That's fine I'll open up the code again tonight when I get home from work and make this functionality available for all nullable types and then push the change to my PR. Hang tight ;)

insert item components

zacbraddy · 2018-10-02T22:23:36Z

Ok mate, I've included that functionality that I added to text nullable types to all nullable types. In doing so I also went ahead and cleaned up a little bit of that component as there were some functions that were only being used in zombie code or not at all in the project. Once I'd cleaned all those up I found that there wasn't as much of a need for the giant if-then-else chain we had. I know it looks like I've changed a lot of code but if you look at the diff type by type and compare to my code you should find that the only significant differences are that I've added the OnFocus and OnChange on to each input that we're using. Let me know if there are any other changes required ;)

hasura-bot · 2018-10-02T22:37:46Z

Review app available at: https://hge-ci-pull-589.herokuapp.com

shahidhk · 2018-10-03T05:08:01Z

@zacbraddy Thanks for your contribution ✨. Works as expected for fields marked Null.

Do you think it will be easy to add the same behaviour to Default also? i.e. If we start typing in a field which has a default value marked, the input box radio button should be selected.

@praveenweb What do you think? I know it's out of scope for the current issue. But would be nice to have.

praveenweb · 2018-10-03T08:48:02Z

@zacbraddy - Thanks. The code refactor looks nice. The giant if-else being rewritten to switch case was neat. The unused code you mentioned was used before when there was a Rich Text Editor functionality for text fields. We had commented out that now, except for this function.

As @shahidhk mentioned in the above comment, can you also add the same behaviour if the field is Default also? I think you are conditionally allowing it if its nullable and not auto-increment, so you can also add if isDefault or something? Could be a simple change, i assume.

zacbraddy · 2018-10-03T09:03:39Z

@praveenweb @shahidhk Yeah guys I can absolutely add something like that. The change isn't massive, however, it's more the "How things should work?" that makes doing it complex. Am I correct in saying that a field can be nullable as well as having a default. If this is the case which should take precedence when the text changes. The questions that need answering are below. I've guessed at the ones I could I've left the ones I'm unsure of for you to answer.

A field is not nullable or defaulted
No functionality applied, the radio boxes stay the same regardless of changes.

A field is nullable, but not defaulted or auto incremented
onFocus:

If there is a value in the field then put the radio to the left on and turn off the nullable radio
onChange:
If length greater than 0 then the left radio is turned on and the nullable radio is turned off
If length 0 then the left radio is turned off and the right radio is turned on

A field is nullable, and auto incremented (implicit default implied here)

No functionality applied, the radio boxes stay the same regardless of changes.

A field is nullable, but not auto incremented and defaulted

???
Profit.

praveenweb · 2018-10-03T11:03:22Z

@zacbraddy

Am I correct in saying that a field can be nullable as well as having a default

Yes this scenario seems to be possible when you want a field to assume default value and accept null values too when required.

IMO, it makes sense to prioritise default over null if both were enabled for the field while it is created. Here is the screenshot which depicts the issue. I think this occurs because null condition is valid.

Now if at all user wants to have control on the radio button selection he can use arrow keys to navigate between radio buttons.

Now when you have a field with only default option selected. User can tab through the UI and start typing on the field. Now when this happens default radio button needs to be unselected and the input box radio button needs to be selected accordingly.

value is empty

zacbraddy · 2018-10-03T20:34:21Z

There you go, as requested 😃

shahidhk · 2018-10-04T14:50:42Z

Awesome @zacbraddy 😎

Looks like some tests are failing for console. @praveenweb @karthikvt26 Can you help out @zacbraddy to fix the tests?

zacbraddy · 2018-10-05T09:10:41Z

Oh bugger, it's cool I'll look into the failing tests and see if I can fix them. I'm going on holiday for a long weekend this weekend so I won't be able to look at them until next week. But if there's no urgency on this then I'm happy to take a look into fixing these when I get back.

shahidhk · 2018-10-09T06:43:52Z

@wawhal Can you quickly check why the tests are failing here and comment with a possible fix?

hasura-bot · 2018-10-09T10:00:18Z

Review app available at: https://hge-ci-pull-589.herokuapp.com

praveenweb · 2018-10-03T08:50:07Z

console/src/components/Services/Data/TableInsertItem/InsertItem.js

+        onClick: clicker,
+        onChange: e => {
+          const textValue = e.target.value;
+          if (isNullable && !isAutoIncrement) {


Also make the radio checked, if the field has a default value. So add another condition?

shahidhk · 2018-10-09T10:18:56Z

@zacbraddy Good news, the tests have been fixed. Finally we can merge this 🎉 👏

Thanks for the contribution 🙏. You'll receive a comment from our bot to fill up your address etc. so that we can send some goodies. Don't forget to fill that when you're back from your vacation. 😄

shahidhk

LGTM

hasura-bot · 2018-10-09T10:20:31Z

Review app https://hge-ci-pull-589.herokuapp.com is deleted

hasura-bot · 2018-10-09T10:20:32Z

Beep boop! 🤖

Whoa! 🎉 🎉 💃

Awesome work @zacbraddy! 💪 🏆 All of us at Hasura ❤️ what you did.

We would love to send you some swag! 👕 Please allow us to do so by filling this form.

If you have any questions, ask us on our Discord & Twitter!

Thanks again 🤗

zacbraddy · 2018-10-09T13:17:16Z

Ah just got back from holiday, sorry to leave you with the tests but thanks for doing that for me @shahidhk was fun working with you guys, you might see me again. Cheers!

shahidhk · 2018-10-10T06:03:06Z

You're always welcome!

cloudflare-workers-and-pages · 2022-02-25T13:36:25Z

Deploying with Cloudflare Pages

Latest commit:	`80c3846`
Status:	🚫 Build failed.

View logs

## Description This adds the smallest possible changes to allow the use of different options when resolving metadata. The concrete idea is this: - each crate (`metadata-resolve`, `schema` etc) will have a type for their flags (if any), but don't care where they come from - when we need to expose these experimental features through the engine (ie, to test requests in e2e tests or something), the engine will parse an env var such as `EXPERIMENTAL_FEATURES=allow_boolean_expression_types` and then use it to construct `MetadataResolveFlags`, `SchemaFlags`, etc. Functional no-op. V3_GIT_ORIGIN_REV_ID: 64ac66569f4e31829a9117c64d2e0e265d7a7303

Fixes hasura#545 - Made it so that with a numllable text field if you…

1d45aef

… enter something it selects the left radio button.

shahidhk requested a review from praveenweb October 1, 2018 09:54

shahidhk added the c/console Related to console label Oct 1, 2018

praveenweb added the s/do-not-merge Do not merge this pull request to master label Oct 2, 2018

Added Null defaulting functionality to all data types and cleaned up the

cce6f3f

insert item components

shahidhk removed the s/do-not-merge Do not merge this pull request to master label Oct 3, 2018

Added functionality for default to be automatically selected when input

006dc83

value is empty

shahidhk added 3 commits October 4, 2018 10:10

Merge branch 'master' into fix-for-insert-nullable-fields

52e916e

Merge branch 'master' into fix-for-insert-nullable-fields

01705dd

Merge branch 'master' into fix-for-insert-nullable-fields

4507ab6

Merge branch 'master' into fix-for-insert-nullable-fields

5231ee5

praveenweb mentioned this pull request Oct 9, 2018

remove an unnecessary test from console #681

Merged

21 tasks

Merge branch 'master' into fix-for-insert-nullable-fields

9c7d66d

praveenweb approved these changes Oct 9, 2018

View reviewed changes

shahidhk added the hacktoberfest label Oct 9, 2018

shahidhk approved these changes Oct 9, 2018

View reviewed changes

shahidhk merged commit 4f2fd73 into hasura:master Oct 9, 2018

zacbraddy deleted the fix-for-insert-nullable-fields branch October 9, 2018 13:17

Allymahmoud mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities Allymahmoud/graphql-engine#234

Open

Made it so that nullable radio defaults correctly (fix #545) #589

Made it so that nullable radio defaults correctly (fix #545) #589

Uh oh!

Conversation

zacbraddy commented Sep 30, 2018

Description

Type

Checklist:

Uh oh!

CLAassistant commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasura-bot commented Sep 30, 2018

Uh oh!

karthikvt26 commented Oct 1, 2018

Uh oh!

praveenweb commented Oct 2, 2018

Uh oh!

praveenweb commented Oct 2, 2018

Uh oh!

zacbraddy commented Oct 2, 2018

Uh oh!

zacbraddy commented Oct 2, 2018

Uh oh!

hasura-bot commented Oct 2, 2018

Uh oh!

shahidhk commented Oct 3, 2018

Uh oh!

praveenweb commented Oct 3, 2018

Uh oh!

zacbraddy commented Oct 3, 2018

Uh oh!

praveenweb commented Oct 3, 2018

Uh oh!

zacbraddy commented Oct 3, 2018

Uh oh!

shahidhk commented Oct 4, 2018

Uh oh!

zacbraddy commented Oct 5, 2018

Uh oh!

shahidhk commented Oct 9, 2018

Uh oh!

hasura-bot commented Oct 9, 2018

Uh oh!

praveenweb Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

shahidhk commented Oct 9, 2018

Uh oh!

shahidhk left a comment

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented Oct 9, 2018

Uh oh!

hasura-bot commented Oct 9, 2018

Uh oh!

zacbraddy commented Oct 9, 2018

Uh oh!

shahidhk commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages bot commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CLAassistant commented Sep 30, 2018 •

edited

Loading

shahidhk commented Oct 10, 2018 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 25, 2022 •

edited

Loading