Custom Fields: Editor UI #13863
Custom Fields: Editor UI #13863
Conversation
…o it's usable in non-product context.
|
|
| var containsHTML: Bool { | ||
| let stripped = self.strippedHTML | ||
| return stripped != self | ||
| } |
There was a problem hiding this comment.
@hichamboushaba and I discussed back and forth on this whether to display Aztec automatically or not when the content is HTML, and this function is meant to support that.
This function is not yet used in any part of the PR since the latest decision is to show a button to show Aztec separately, but might be needed later on. It can also be deleted later once we finalize everything.
There was a problem hiding this comment.
It would be nice to have a unit test for this property
There was a problem hiding this comment.
With the latest conversation I think we want to make aztec editor available to all kind of text, and let user choose to use it instead of automatically selecting it for them, so this becomes unnecessary anymore. Removed in 45624f2
There was a problem hiding this comment.
I reviewed the PR and this is what I found:
Missing fields
I noticed that, for some reason, one custom field doesn't appear in one order, while in another order the same custom field appears. The only difference is that the order where the custom field doesn't appear is in Processing status, while the other one is completed. After adding an extra custom field, then it started to appear. Any guess why this is happening? The value of the custom field is the same between those orders.
| Expected custom field | What I see in that order |
|---|---|
 |
 |
Fields editing
When you tap on one field in the list, specifically you tap on title or description, they do not open at all. This often happens with custom fields containing long text. Sometimes, when you press on them, they appear to be in a "selectable" status, but I cannot navigate to the details until I press on the borders of the cell.
| var containsHTML: Bool { | ||
| let stripped = self.strippedHTML | ||
| return stripped != self | ||
| } |
There was a problem hiding this comment.
It would be nice to have a unit test for this property
| import SwiftUI | ||
|
|
||
| struct AztecEditorView: UIViewControllerRepresentable { | ||
| @Binding var html: String | ||
|
|
||
| func makeUIViewController(context: Context) -> AztecEditorViewController { | ||
| let viewProperties = EditorViewProperties(navigationTitle: "Navigation title", /* todo replace string */ | ||
| placeholderText: "placeholder text", /* todo replace string */ | ||
| showSaveChangesActionSheet: true) | ||
|
|
||
| let controller = AztecEditorViewController( | ||
| content: html, | ||
| product: nil, | ||
| viewProperties: viewProperties, | ||
| isAIGenerationEnabled: false | ||
| ) | ||
|
|
||
| return controller | ||
| } | ||
|
|
||
| func updateUIViewController(_ uiViewController: AztecEditorViewController, context: Context) { | ||
| // Update the view controller if needed | ||
| } | ||
| } |
There was a problem hiding this comment.
This struct looks like something that could be used elsewhere in the future, so I strongly suggest moving it to a different file.
There was a problem hiding this comment.
I'm working on this on a separate PR, here #13925
| struct CustomFieldEditorView: View { | ||
| @State private var key: String | ||
| @State private var value: String | ||
| @State private var isModified: Bool = false |
There was a problem hiding this comment.
I think hasUnsavedChanges is clearer and is already used throughout the app.
There was a problem hiding this comment.
Sounds good! Updated in 22e7f3e
| private struct RichTextEditor: View { | ||
| @Binding var html: String | ||
| @State private var isModified: Bool = false | ||
| @Environment(\.presentationMode) var presentationMode | ||
|
|
||
| var body: some View { | ||
| NavigationView { | ||
| AztecEditorView(html: $html) | ||
| .toolbar { | ||
| ToolbarItem(placement: .navigationBarLeading) { | ||
| Button { | ||
| presentationMode.wrappedValue.dismiss() | ||
| } label: { | ||
| Text("Cancel") // todo-13493: set String to be translatable | ||
| } | ||
| } | ||
| ToolbarItem(placement: .navigationBarTrailing) { | ||
| Button { | ||
| // todo-13493: implement save action | ||
| presentationMode.wrappedValue.dismiss() | ||
| } label: { | ||
| Text("Done") // todo-13493: set String to be translatable | ||
| } | ||
| .disabled(!isModified) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I would also move this to a separate file since it could be reused.
There was a problem hiding this comment.
Also worked on separate PR #13925
| static let sectionSpacing: CGFloat = 16 | ||
| static let subSectionsSpacing: CGFloat = 8 | ||
| static let cornerRadius: CGFloat = 8 | ||
| static let inputInsets = EdgeInsets(top: 8, leading: 5, bottom: 8, trailing: 5) |
There was a problem hiding this comment.
This is the first time I've seen 5 used as padding in the app. Are we sure it shouldn't have been 6 or 8?
There was a problem hiding this comment.
For more context on this, I was looking at existing UI pattern in the app that shows more than 1 input fields on one screen (key/value input in our case).
That layout is surprisingly rare in our app, and I was able to find it in the Contact Support screen:
The layout here is pretty much a copy from there, here:
There was a problem hiding this comment.
Got it :D I think this is an "error" also there probably
| /// - key: The key for the custom field | ||
| /// - value: The value for the custom field | ||
| /// - isReadOnlyValue: Whether the value is read-only or not. To be used if the value is not string but JSON. | ||
| init(key: String, value: String, isReadOnlyValue: Bool = false) { |
There was a problem hiding this comment.
Why not pass the CustomFieldViewModel as input here? Any specific reason?
There was a problem hiding this comment.
This editor will also be used when adding a new custom field, and my thinking was that we don't need all the extra information in CustomFieldViewModel for this purpose, and instead just pass empty strings instead. wdyt?
There was a problem hiding this comment.
I think it's fine to start with, but consider you should not be able to test the editor logic.
There was a problem hiding this comment.
Good point here, I'll keep in mind for future refactor I think!
| title: customField.title, | ||
| content: customField.content, | ||
| contentURL: customField.contentURL) | ||
| if isEditable { |
There was a problem hiding this comment.
If not editable, it would be nice to show an alert, explaining why
There was a problem hiding this comment.
isEditable here is simply a flag that contains the feature flag check while we're still developing. Once we're done, this will be removed as any field will be tappable (I see that it's confusing and probably should be isTappable instead of isEditable).
We do have a special type (JSON value) that can be tapped but can't be edited (refer to isReadOnlyValue param in CustomFieldEditorView). I agree we should let users know why this is not editable, whether with alert or a sort of label above the text box.
Interesting observation! There is no change in this PR in terms of which custom fields are shown, it's simply using all the metadata that the order has: So, whatever the issue is, it already exists prior to this PR. I'll see if I can replicate it and put it in a separate issue. |
Thanks! This was inconsistent to me but I was able to replicate from time to time. I added fdf7def that should fix it. |
|
@pmusolino Thank you for the reviews! Aside from separating the Aztec view to their own file, which will be handled in this PR, I believe I've responded to all the review items. Good for another round of review! |
|
Version |
Part of: #13493
Description
This PR adds the UI for editing custom fields.
These items are missing and should be added separately when working on functionality:
Steps to reproduce
Testing information
I tested this on Simulator by going through the Order > Custom Fields flow as listed above.
Screenshots
RELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: