Add TextField, SecureField, and a progress document #116

j-f1 · 2020-06-28T19:15:40Z

Currently, it seems that changing the text binding doesn’t actually affect the rendered input. I’m not sure why this happens, but I’m guessing it’s because the value attribute isn’t being set as <input>.value.

I’ve added two TextFields to the demo, which are supposed to have their values synced. Pressing enter in the second one will increment the commit counter.

j-f1 · 2020-06-28T19:19:13Z

Looks like Danger is failing at least because it can’t access the GitHub token since this PR comes from a fork.

MaxDesiatov · 2020-06-28T20:02:57Z

Thanks for the PR, I'll have a look a bit later today! danger-lint failing is a known issue, I'll turn it off in main so that this doesn't confuse anyone anymore.

MaxDesiatov · 2020-06-28T20:44:44Z

progress.md

@@ -0,0 +1,137 @@
+)# Progress


This is a file I should've created from the start, thank you for doing this! Could you also create a docs directory and move it there? Also, seems that ) in the beginning is not needed:

Suggested change

)# Progress

# Progress

How detailed should this be? I originally had it in a list format which could’ve displayed individual method names but I figured the table would give people a good overview of what is usable and where they can jump in.

Oh, what is currently there already looks great. Maybe I'd tweak table headers, they look kind of weird as empty thin rows, but don't sweat it. I think the API comparison should be automated at some point, but for the list of basic fundamental views this should be more than enough from my perspective, unless you think otherwise.

Hey @carson-katri, I'd be happy to hear your opinion on this, what do you think?

I can explore an automated approach. In the meantime, you could do the table with HTML to get rid of the header if you wanted to fix them.

I don't think headers are that important, so wouldn't mind if they are kept as is in this PR.

MaxDesiatov · 2020-06-28T21:25:34Z

I think the text binding issues may be caused by bugs in the State/Binding code, investigating now...

MaxDesiatov · 2020-06-28T21:55:58Z

Turns out it's not a state/binding issue, but a more prosaic oversight that's fixed in #117.

j-f1 · 2020-06-28T22:01:36Z

Is there a way to hook into the update function? I need to update the actual value property of the input element, not the value attribute.

MaxDesiatov · 2020-06-28T22:02:52Z

Is there a signifcant difference? I've tested your branch locally with #117 applied on top of it and it worked, or am I missing something?

j-f1 · 2020-06-28T22:05:40Z

If you type in both fields they will stop updating.

MaxDesiatov · 2020-06-28T22:06:11Z

huh, good point, on it...

MaxDesiatov · 2020-06-28T23:03:03Z

I updated #117 to update properties, not attributes, so should work now when combined with this PR.

j-f1 · 2020-06-29T00:21:46Z

Is it intentional that VStack can have a max of 10 children?

j-f1 · 2020-06-29T00:31:00Z

Updated. ~~Note that the newly added text field is still editable despite that not being the intention.~~

carson-katri · 2020-06-29T00:31:01Z

@j-f1 This is a limitation of the ViewBuilder, and is present in SwiftUI as well. ViewBuilder takes in arguments and builds a TupleView which is generated to have up to 10 children via buildBlock functions. If you use ForEach or a Group you can go past that limit.

j-f1 · 2020-06-29T02:18:58Z

OK cool I reviewed native SwiftUI and the text field’s behavior is the same there 🤷

j-f1 · 2020-06-29T02:24:37Z

I tried to implement the other TextField initializer but it looks like Foundation (and therefore the required Formatter) are not yet available?

j-f1 · 2020-06-29T02:39:48Z

I dislike the way the SecureField is largely a copy-paste of TextField but I can’t think of a better way since you can’t extend structs.

carson-katri · 2020-06-29T02:44:00Z

Could you do a protocol that has all the fields and a isSecure flag?

MaxDesiatov · 2020-06-29T08:34:50Z

Foundation is available if you build the package in a special way, but I'd like to avoid it as much as possible. It's a huge dependency, and I'm afraid that even when SwiftWasm has dead code elimination working properly, I'm not sure we'll be able to strip out unused bits of Foundation cleanly, especially due to how big it is and how it also proxies a lot of stuff to CoreFoundation. On the other hand, without Foundation we won't get the Date type, which would be frustrating.

We need to tweak carton so that it links Foundation automatically, but I think for now, please leave out the bits that require Foundation, and maybe add a comment about what's missing.

j-f1 · 2020-06-29T12:39:09Z

Could you do a protocol that has all the fields and a isSecure flag?

I gave that a try and it didn’t cut down on the duplication a ton.

MaxDesiatov · 2020-06-29T14:38:07Z

Sources/TokamakCore/Views/TextField.swift

+    commitAction = onCommit
+  }
+
+  // Currently missing feature: use a Formatter to control the value of the TextField


suggestion: I think it would make sense to highlight this with a FIXME.

Suggested change

// Currently missing feature: use a Formatter to control the value of the TextField

// FIXME:

// Currently missing feature: use a Formatter to control the value of the TextField

MaxDesiatov

Overall LGTM other than a missing FIXME. I agree that SecureField and TextField renderer bits generalized with a protocol probably won't save enough lines of code, but I don't have a strong opinion on whether that protocol should exist, I'll defer to @carson-katri as he suggested it 🙂

carson-katri · 2020-06-29T14:46:16Z

SwiftUI doesn't use a protocol, and if it's good enough for Apple then it's good enough for me 😄

MaxDesiatov

Thanks! There's also a conflict in Sources/TokamakDemo/main.swift to be resolved.

MaxDesiatov

Seems legit 👍

Add TextField

f076dc8

Add progress doc

95e4b0e

MaxDesiatov added the SwiftUI compatibility Tokamak API differences with SwiftUI label Jun 28, 2020

MaxDesiatov reviewed Jun 28, 2020

View reviewed changes

Move the progress file to docs/

d33b6fe

MaxDesiatov requested a review from carson-katri June 28, 2020 21:13

MaxDesiatov mentioned this pull request Jun 28, 2020

Update DOM node properties and listeners on renderer update #117

Merged

MaxDesiatov changed the title ~~Add TextField~~ Add TextField and a progress document Jun 28, 2020

Merge remote-tracking branch 'upstream/main' into text-field

924c4e7

Add more tests

2a8db1b

Add SecureField

07baeec

j-f1 changed the title ~~Add TextField and a progress document~~ Add TextField, SecureField, and a progress document Jun 29, 2020

Add unimplemented feature

abcce81

MaxDesiatov reviewed Jun 29, 2020

View reviewed changes

add FIXME

71efdc6

j-f1 marked this pull request as ready for review June 29, 2020 16:13

MaxDesiatov requested changes Jun 29, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/main' into text-field

ffa61db

MaxDesiatov approved these changes Jun 29, 2020

View reviewed changes

MaxDesiatov merged commit b2b5ec2 into TokamakUI:main Jun 29, 2020

j-f1 deleted the text-field branch June 29, 2020 16:54

j-f1 mentioned this pull request Jun 29, 2020

Add markdown doc with a list of all available views #21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TextField, SecureField, and a progress document #116

Add TextField, SecureField, and a progress document #116

j-f1 commented Jun 28, 2020 •

edited

j-f1 commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

MaxDesiatov Jun 28, 2020

j-f1 Jun 28, 2020

MaxDesiatov Jun 28, 2020

carson-katri Jun 28, 2020

MaxDesiatov Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

j-f1 commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

j-f1 commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

j-f1 commented Jun 29, 2020

j-f1 commented Jun 29, 2020 •

edited

carson-katri commented Jun 29, 2020 •

edited

j-f1 commented Jun 29, 2020

j-f1 commented Jun 29, 2020

j-f1 commented Jun 29, 2020

carson-katri commented Jun 29, 2020

MaxDesiatov commented Jun 29, 2020

j-f1 commented Jun 29, 2020

MaxDesiatov Jun 29, 2020

MaxDesiatov left a comment

carson-katri commented Jun 29, 2020

MaxDesiatov left a comment

MaxDesiatov left a comment

	// Currently missing feature: use a Formatter to control the value of the TextField
	// FIXME:
	// Currently missing feature: use a Formatter to control the value of the TextField

Add TextField, SecureField, and a progress document #116

Add TextField, SecureField, and a progress document #116

Conversation

j-f1 commented Jun 28, 2020 • edited

j-f1 commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

MaxDesiatov Jun 28, 2020

Choose a reason for hiding this comment

j-f1 Jun 28, 2020

Choose a reason for hiding this comment

MaxDesiatov Jun 28, 2020

Choose a reason for hiding this comment

carson-katri Jun 28, 2020

Choose a reason for hiding this comment

MaxDesiatov Jun 28, 2020

Choose a reason for hiding this comment

MaxDesiatov commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

j-f1 commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

j-f1 commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

MaxDesiatov commented Jun 28, 2020

j-f1 commented Jun 29, 2020

j-f1 commented Jun 29, 2020 • edited

carson-katri commented Jun 29, 2020 • edited

j-f1 commented Jun 29, 2020

j-f1 commented Jun 29, 2020

j-f1 commented Jun 29, 2020

carson-katri commented Jun 29, 2020

MaxDesiatov commented Jun 29, 2020

j-f1 commented Jun 29, 2020

MaxDesiatov Jun 29, 2020

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

carson-katri commented Jun 29, 2020

MaxDesiatov left a comment

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

j-f1 commented Jun 28, 2020 •

edited

j-f1 commented Jun 29, 2020 •

edited

carson-katri commented Jun 29, 2020 •

edited