Support hooks OR atom state management in component previews #20053
+172
−123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilmotta
added
the
E:Mobile Refactor Quo2 Preview To Best Practices
ilmotta
commented
May 15, 2024
| [{:keys [label state set-state] :as args}] | ||
| (let [theme (quo.theme/use-theme) | ||
| label (or label (key->boolean-label (:key args))) | ||
| field-cursor (when-not (fn? set-state) |
There was a problem hiding this comment.
FYI: As a temporary solution until we update all component previews, we check if set-state is a function and use that instead of the Reagent atom. This is how we support both APIs.
I didn't bother making things pretty and copy & pasted more because most of this will go away anyway.
Jenkins BuildsClick to see older builds (4)
|
jo-mut
approved these changes
May 17, 2024
|
@flexsurfer, @Parveshdhull, @J-Son89, I'm kindly asking you to review again this PR because it's a quick and safe move forward and I'd like to get at least 2 approvals. Hopefully reviewing it doesn't take too much of your time. Thanks! |
J-Son89
approved these changes
May 17, 2024
ilmotta
force-pushed
the
ilmotta/support-both-apis-for-managing-preview-state
branch
from
8574572
to
cbd7985
Compare
Parveshdhull
approved these changes
May 17, 2024
83% of end-end tests have passedFailed tests (7)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
ilmotta
force-pushed
the
ilmotta/support-both-apis-for-managing-preview-state
branch
from
cbd7985
to
4c40c62
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR changes the underlying implementation of component previews to support receiving either
state/set-statefromuse-stateor a Reagent atom, thus allowing us to gradually change preview namespaces to use hooks instead of having to refactor all at once in a gigantic PR.I tested all types of preview fields, including multi-select and it's all working. I changed only two component previews to keep this PR small because my goal here is not to rewrite preview namespaces to use hooks (we better do in separate PRs).
Areas that may be impacted
None. Only quo previews changed.
Steps to test
Only the components
counter.stepandselectors.reactpreviews were adapted touse-stateif you want to check them out and prove it all works.Demo of the refactored component using multi-select field:
previews.webm
status: ready