Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Custom Fields] HTML toggle #12670

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Sep 23, 2024

Closes: #12657

Description

This PR updates the logic of the editor to always offer a toggle to switch between the text editor and the Aztec one.

We decided to take this approach instead of showing the Aztec editor for HTML values by default for two reasons:

  1. The Aztec editor can sometimes remove some parts (like the DOCTYPE tag) and also has some issues with inline styling, so it's best to have an explicit action from the user before using it.
  2. This approach is also more flexible.

For more information, check this discussion: p1725529996823859/1724862433.885169-slack-C03L1NF1EA3

Steps to reproduce

  1. Make sure you have an order with at least one custom field.
  2. Open the order on the app.
  3. Tap on "View custom fields"
  4. Edit or add a new field.

Testing information

  • Test the behavior of the HTML toggle.
  • Confirm that the "Source" button is not shown on the Aztec toolbar.
  • Type some long text on the field, and confirm scrolling works as you would expect.

The tests that have been performed

Same as the above.

Images/gif

Screen_recording_20240923_163725.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@hichamboushaba hichamboushaba added type: enhancement A request for an enhancement. feature: order details Related to order details. feature: product details Related to adding or editing products, includes product settings. labels Sep 23, 2024
@hichamboushaba hichamboushaba added this to the 20.6 milestone Sep 23, 2024
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 23, 2024

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commitf24ca15
Direct Downloadwoocommerce-wear-prototype-build-pr12670-f24ca15.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 23, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commitf24ca15
Direct Downloadwoocommerce-prototype-build-pr12670-f24ca15.apk

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2024

Codecov Report

Attention: Patch coverage is 13.41463% with 71 lines in your changes missing coverage. Please review.

Project coverage is 40.63%. Comparing base (1212943) to head (f24ca15).
Report is 4 commits behind head on trunk.

Files with missing lines Patch % Lines
.../android/ui/compose/component/aztec/AztecEditor.kt 0.00% 70 Missing ⚠️
...customfields/editor/CustomFieldsEditorViewModel.kt 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #12670      +/-   ##
============================================
- Coverage     40.64%   40.63%   -0.02%     
+ Complexity     5674     5673       -1     
============================================
  Files          1229     1229              
  Lines         69178    69214      +36     
  Branches       9579     9587       +8     
============================================
+ Hits          28119    28125       +6     
- Misses        38475    38506      +31     
+ Partials       2584     2583       -1     
Flag Coverage Δ
40.63% <13.41%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hichamboushaba hichamboushaba force-pushed the issue/12657-custom-fields-aztec-toggle branch from 5919089 to 1a15a62 Compare September 23, 2024 14:30
This avoids having an editor that spans longer than the available space making the whole UI scrollable
Comment on lines +324 to +372
aztec.visualEditor.addOnLayoutChangeListener { _, _, _, _, _, _, _, _, _ ->
// Because the editors could have different number of lines, we don't set the minLines
// of the source editor, so we set the minHeight instead to match the visual editor
sourceEditorMinHeight = aztec.visualEditor.height
}

aztec.visualEditor.doAfterTextChanged {
if (!state.isHtmlEditorEnabled) return@doAfterTextChanged
state.updateContent(aztec.visualEditor.toHtml())
}
aztec.sourceEditor?.doAfterTextChanged {
val sourceEditor = aztec.sourceEditor
if (state.isHtmlEditorEnabled || sourceEditor == null) return@doAfterTextChanged
state.updateContent(sourceEditor.getPureHtml())
}

val focusChangeListener = OnFocusChangeListener { _, focused ->
focusState.value = focused
}
aztec.visualEditor.onFocusChangeListener = focusChangeListener
aztec.sourceEditor?.onFocusChangeListener = focusChangeListener

viewsHolder.layout
},
update = {
if (aztec.visualEditor.isInCalypsoMode != calypsoMode) {
aztec.visualEditor.isInCalypsoMode = calypsoMode
aztec.sourceEditor?.setCalypsoMode(calypsoMode)
}

if (sourceEditorMinHeight != aztec.sourceEditor?.minHeight) {
aztec.sourceEditor?.minHeight = sourceEditorMinHeight
}
if (minLines != -1 && minLines != aztec.visualEditor.minLines) {
aztec.visualEditor.minLines = minLines
}
if (maxLines != Int.MAX_VALUE && maxLines != aztec.visualEditor.maxLines) {
aztec.visualEditor.maxLines = maxLines
aztec.sourceEditor?.maxLines = maxLines
}

if (aztec.visualEditor.label != label) {
aztec.visualEditor.label = label
aztec.sourceEditor?.label = label
}
},
modifier = modifier
.bringIntoViewRequester(bringIntoViewRequester)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

All of this part wasn't touched, it's shown as edited just because it was wrapped in a key function.

Column(
modifier = Modifier
.verticalScroll(rememberScrollState())
.heightIn(max = max(maxHeight, 320.dp))
Copy link
Member Author

@hichamboushaba hichamboushaba Sep 23, 2024

Choose a reason for hiding this comment

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

We set the maximum height of the Column to the available maxHeight, unless it's less than 320.dp, this second condition is needed to avoid having an overlap between the key and value editors when maxHeight is small, for example when on landscape and the keyboard is shown on a phone.

@hichamboushaba hichamboushaba marked this pull request as ready for review September 23, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: order details Related to order details. feature: product details Related to adding or editing products, includes product settings. type: enhancement A request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Custom Fields] Update the Aztec integration to use a toggle
4 participants