Improve text field input #27

W1W1-M · 2022-08-17T12:46:29Z

Hi @sindresorhus
This PR is a first draft for issue #3 Validate text fields

I first tried a border color for the hex text field but it interfered with blue border when the text field is focused. Instead I added a SF symbol image that switches depending if the field is valid.

The implemented colorChecker class uses ObservedObject and the onChange modifier to run on user input, it also trims unwanted characters.

The code still needs to be implemented for hsl, rgb & lch text inputs. Also needs cleaning, optimizing & testing but I wanted an initial 👍 before going ahead.

Regards

PS : I have more experience with iOS app development, this is helping me get to grips with Mac app development 😄

…ngs & checking methods. Add mark or checkmark image for feedback on valid text input. Fix SwiftLint warnings.

sindresorhus · 2022-08-17T20:08:44Z

Thanks for working on this. I don't think this is the right approach though.

Here's what I'm thinking:

Abstract all of the hexColorView type views into a single (general) view that can be reused for each of the color inputs.
The validation logic is already implemented, if let newColor = NSColor(hexString: hexColor.trimmingCharacters(in: .whitespaces)) succeeds, it means the color is valid. If not, you can show the invalid state.

I'm also not a big fan of the symbols. We don't need a success indication, just a failure one. So I think just coloring the text red if it's invalid is enough.

Color Picker/ColorPickerScreen.swift

W1W1-M · 2022-08-17T20:27:02Z

Abstract all of the hexColorView type views into a single (general) view that can be reused for each of the color inputs.

Ok, I'll work on that and update accordingly

The validation logic is already implemented, if let newColor = NSColor(hexString: hexColor.trimmingCharacters(in: .whitespaces)) succeeds, it means the color is valid. If not, you can show the invalid state.

I didn't realize all the logic was already there 😅 Thanks for pointing this out !

I'm also not a big fan of the symbols. We don't need a success indication, just a failure one. So I think just coloring the text red if it's invalid is enough.

Ok, simple and straightforward is good 😄 I'll update accordingly

…r text input validation. Keep SwiftLint warning fixes.

…ndition to change text color if color code is invalid. Add condition to limit character input. Add textColor to NativeTextField for use in updateNSView method. Update other ColorViews to use textColor.

W1W1-M · 2022-08-18T20:40:55Z

So this is still WIP, but some feedback to know if this is more inline with the proposed approach would be nice 😃

As for hex codes: they are limited to a small set of characters and also in length, I've added some code to filter and trim hex inputs (ex: #123456, FFFFFF, etc). This helps users input only valid hex codes but also makes the input rigid and removes the need to indicate (in red) an invalid input. Maybe a compromise of just trimming excess characters without filtering is a good balance ?

… Add hsl ColorInputView to ColorPickerScreen. Update comments.

Color Picker/ColorPickerScreen.swift

sindresorhus · 2022-08-19T13:00:08Z

Color Picker/ColorPickerScreen.swift

+        case rgb = "RGB"
+        case lch = "LCH"
+    }
+    var colorKeyboardShortcut: KeyEquivalent {


This should be on a private extension on the ColorFormat enum, placed in this file.

I've moved keyboardShortcut into an extension of ColorFormat within ColorPickerScreen.

Would it be better to centralise this extension in Constants as not to have class extensions in different files ? or is it better to keep here, in order to limit access control of extension to this file only ?

sindresorhus · 2022-08-19T13:02:41Z

So this is still WIP, but some feedback to know if this is more inline with the proposed approach would be nice 😃

Yes, it's the correct direction.

sindresorhus · 2022-08-19T13:04:08Z

This helps users input only valid hex codes but also makes the input rigid

Trimming whitespace is ok, but I don't think we should prevent some characters. Users will not understand that it's because it's not valid Hex. I think it's better to just make it red in that case.

…rText filter.

…rView.

W1W1-M · 2022-08-19T21:49:53Z

On user color input, updateColorsFromPanel method needs to be called to update other color codes. This method is currently in ColorPickerScreen. In order to be able to call it from each ColorInputView I believe it needs to be moved to AppState EnvironmentObject or some other class, along with required properties (as @Published properties) ?
Or maybe there is smarter way to call it ?
Or does it need a bigger rethink as the existing comment indicates:

TODO: Find a better way to handle this.

What's your opinion on this @sindresorhus ?

@published

…properties. Move updateColorsFromPanel method to AppState. Refactor ColorInputView & ColorPickerScreen to use @published properties from AppState. Delete unused textColor variable. Update comments.

sindresorhus · 2022-08-30T09:22:03Z

Color Picker/AppState.swift

+    @Published var hslColor = ""
+    @Published var rgbColor = ""
+    @Published var lchColor = ""
+    @Published var isPreventingUpdate = false


Why are these moved to AppState? AppState is for state that is required globally. This state is only needed for picker screen. I think you can just pass updateColorsFromPanel to the ColorPickerScreen view.

sindresorhus · 2022-08-30T09:24:43Z

Color Picker/ColorPickerScreen.swift

@@ -127,22 +127,149 @@ private struct BarView: View {
 	}
 }

+struct ColorInputView: View {
+    @EnvironmentObject private var appState: AppState
+    @State private var textColor: Color = .primary


It's better to use an optional and default to nil.

sindresorhus · 2022-08-30T09:25:36Z

Color Picker/ColorPickerScreen.swift

+                textColor: textColor
+            )
+                .controlSize(.large)
+                .onChange(of: inputColorText) {


Move the body out into a private function on the view. It makes it easier to see just the view logic.

sindresorhus · 2022-08-30T09:26:36Z

Color Picker/ColorPickerScreen.swift

+
+                        if !appState.isPreventingUpdate {
+                            appState.updateColorsFromPanel(excludeLCH: true, preventUpdate: true, color: colorPanel.color)
+                        }


There's a lot of duplicated logic in each case here. Would be good to reduce that.

For example, appState.updateColorsFromPanel could be updated to accept an enum instead of excludeLCH: true type parameters.

sindresorhus · 2022-08-30T09:29:32Z

Color Picker/ColorPickerScreen.swift

+                ColorInputView(
+                    inputColorText: $appState.lchColor,
+                    isTextFieldFocused: $isTextFieldFocusedLCH,
+                    colorPanel: colorPanel,


The color panel is available on the AppState, so you can access it from there instead.

sindresorhus · 2022-08-30T09:29:40Z

Color Picker/ColorPickerScreen.swift

+                    isTextFieldFocused: $isTextFieldFocusedLCH,
+                    colorPanel: colorPanel,
+                    textFieldFontSize: textFieldFontSize,
+                    inputColorType: .lch


This parameter should be first.

sindresorhus · 2022-08-30T09:30:28Z

Color Picker/ColorPickerScreen.swift

-				.keyboardShortcut("l", modifiers: [.shift, .command])
-		}
-	}
-
 	var body: some View {
 		VStack {
 			BarView()
 			if shownColorFormats.contains(.hex) {


You can just loop over the enum instead of manually defining each view. That was the point of extracting it into a reusable view.

sindresorhus · 2022-08-30T09:31:06Z

Color Picker/Utilities.swift

@@ -1021,6 +1021,7 @@ struct NativeTextField: NSViewRepresentable {
 	var isFirstResponder = false
 	@Binding var isFocused: Bool // Note: This is only readable.
 	var isSingleLine = true
+    var textColor: Color


Nitpick: Should be below font.

sindresorhus · 2022-08-30T09:34:54Z

Color Picker/ColorPickerScreen.swift

-	@State private var hexColor = ""
-	@State private var hslColor = ""
-	@State private var rgbColor = ""
-	@State private var lchColor = ""
 	@State private var isTextFieldFocusedHex = false
 	@State private var isTextFieldFocusedHSL = false
 	@State private var isTextFieldFocusedRGB = false
 	@State private var isTextFieldFocusedLCH = false


This could be a single state that accepts an optional ColorFormat.

sindresorhus · 2023-06-20T12:45:47Z

Closing for lack of activity.

Add ColorChecker class. Add colorInputs ObservedObject for color stri…

2430f97

…ngs & checking methods. Add mark or checkmark image for feedback on valid text input. Fix SwiftLint warnings.

W1W1-M mentioned this pull request Aug 17, 2022

Validate text fields #3

Open

sindresorhus reviewed Aug 17, 2022

View reviewed changes

Color Picker/ColorPickerScreen.swift Show resolved Hide resolved

W1W1-M added 2 commits August 18, 2022 13:54

Delete ColorChecker class & ObservedObject. Delete SF symbol for colo…

2d1aafb

…r text input validation. Keep SwiftLint warning fixes.

Add ColorInputView. Setup ColorInputView for hex color inputs. Add co…

59b5e48

…ndition to change text color if color code is invalid. Add condition to limit character input. Add textColor to NativeTextField for use in updateNSView method. Update other ColorViews to use textColor.

W1W1-M marked this pull request as draft August 18, 2022 20:25

W1W1-M added 2 commits August 19, 2022 14:38

Move hsl logic to ColorInputView. Delete hexColorView & hslColorView.…

dbf5bed

… Add hsl ColorInputView to ColorPickerScreen. Update comments.

Isolate textColor logic

99f45b2

sindresorhus reviewed Aug 19, 2022

View reviewed changes

Color Picker/ColorPickerScreen.swift Outdated Show resolved Hide resolved

sindresorhus reviewed Aug 19, 2022

View reviewed changes

W1W1-M added 3 commits August 19, 2022 17:16

Update inputColorType to ColorFormat type. Delete colorType enum.

26509e5

Move colorKeyboardShortcut to ColorFormat extension. Delete inputColo…

1cf5e65

…rText filter.

Move rgb & lch logic to ColorInputView. Delete rgbColorView & lchColo…

32d5826

…rView.

Move isPreventingUpdate bool & color strings to AppState as @published …

98eef57

…properties. Move updateColorsFromPanel method to AppState. Refactor ColorInputView & ColorPickerScreen to use @published properties from AppState. Delete unused textColor variable. Update comments.

sindresorhus requested changes Aug 30, 2022

View reviewed changes

sindresorhus force-pushed the main branch 3 times, most recently from 68fbb92 to cf553c1 Compare December 1, 2022 09:16

sindresorhus force-pushed the main branch from 656418d to 0092873 Compare January 17, 2023 09:12

sindresorhus force-pushed the main branch 3 times, most recently from 1fe36bd to 189b45f Compare April 2, 2023 09:43

sindresorhus closed this Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve text field input #27

Improve text field input #27

W1W1-M commented Aug 17, 2022

sindresorhus commented Aug 17, 2022 •

edited

Loading

W1W1-M commented Aug 17, 2022

W1W1-M commented Aug 18, 2022

sindresorhus Aug 19, 2022 •

edited

Loading

W1W1-M Aug 19, 2022 •

edited

Loading

sindresorhus commented Aug 19, 2022

sindresorhus commented Aug 19, 2022

W1W1-M commented Aug 19, 2022 •

edited

Loading

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus Aug 30, 2022

sindresorhus commented Jun 20, 2023

Improve text field input #27

Improve text field input #27

Conversation

W1W1-M commented Aug 17, 2022

sindresorhus commented Aug 17, 2022 • edited Loading

W1W1-M commented Aug 17, 2022

W1W1-M commented Aug 18, 2022

sindresorhus Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

W1W1-M Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

sindresorhus commented Aug 19, 2022

sindresorhus commented Aug 19, 2022

W1W1-M commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Jun 20, 2023

sindresorhus commented Aug 17, 2022 •

edited

Loading

sindresorhus Aug 19, 2022 •

edited

Loading

W1W1-M Aug 19, 2022 •

edited

Loading

W1W1-M commented Aug 19, 2022 •

edited

Loading