chore: slider button - add error type, blur variant and fix small ui bug #17473
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
J-Son89
changed the title
Jenkins BuildsClick to see older builds (12)
|
J-Son89
commented
Oct 2, 2023
| @@ -22,7 +22,7 @@ | |||
| (defn- track-cover-interpolation | |||
| [track-width thumb-size] | |||
| {:in [0 1] | |||
| :out [(/ thumb-size 2) track-width]}) | |||
| :out [(/ thumb-size 1.8) track-width]}) | |||
J-Son89
changed the title
J-Son89
force-pushed
the
jc/slider-button
branch
from
ce20fbf to
f55476d
Compare
J-Son89
commented
Oct 2, 2023
| @@ -22,7 +22,7 @@ | |||
| (defn- track-cover-interpolation | |||
| [track-width thumb-size] | |||
| {:in [0 1] | |||
| :out [(/ thumb-size 2) track-width]}) | |||
| :out [thumb-size track-width]}) | |||
There was a problem hiding this comment.
I think smth like this would be better, to assure the cover moves together with the right edge of the thumb, so on quick slides it's less likely that it would be visible on the other side:
(defn- track-cover-interpolation
[track-width thumb-size]
{:in [0 1]
:out [thumb-size (+ track-width thumb-size)]})Here's the before [thumb-size track-width] and after [thumb-size (+ track-width thumb-size)] (red is the cover):
There was a problem hiding this comment.
Suggested change
| :out [thumb-size track-width]}) | |
| :out [thumb-size (+ track-width thumb-size)]}) |
There was a problem hiding this comment.
thanks, and I appreciate the very clear images! :)
J-Son89
requested review from
ilmotta,
alexandraB99,
BalogunofAfrica,
mmilad75,
mohsen-ghafouri and
clauxx
and removed request for
ilmotta
clauxx
reviewed
Oct 3, 2023
Comment on lines
+38
to
+40
| :component-container-style (when-not @blur? (:align-items :center)) | ||
| :blur? @blur? | ||
| :show-blur-background? true} |
There was a problem hiding this comment.
If blur is only for the dark theme, should add the blur-dark-only? prop as well.
There was a problem hiding this comment.
imo, this feature would have benefitted from discussion before being merged/moved forward. This blur-dark-only? option ignores that there is also blur-light-only (at least I think there is some cases). Not to mention that there is many incorrect configurations of components possible on the preview screens. The button is a good example of this and a better alternative (imo) is to block incorrect configurations via disabling the descriptors for relevant options. This would of course benefit a lot if Mali gets added to the codebase.
Having this discussion was mentioned here (although I'm not sure if the pr with this feature had got merged before, I assume it had so no big deal):
#17271 (comment)
response - #17271 (comment)
response - #17271 (comment)
imo it should be commonplace to discuss with those interested in the team before we add new features to the preview screens as they tend to effect everyone, and with a team so large it takes a while to make later refactors once a feature starts getting used.
J-Son89
requested review from
FFFra and
tumanov-alex
and removed request for
alexandraB99
J-Son89
force-pushed
the
jc/slider-button
branch
from
f55476d to
a91b987
Compare
|
Thanks @pavloburykh - that's expected but not desirable :). There is no |
0% of end-end tests have passedFailed tests (43)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
Thank you @J-Son89! Could you please rebase the PR as it is quite outdated. After that we will need to re-run e2e as they failed due to a problem of outdated branch. |
J-Son89
force-pushed
the
jc/slider-button
branch
from
a91b987 to
735a319
Compare
|
@pavloburykh - done! apologies I thought I did that this morning. :/ |
86% of end-end tests have passedFailed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (37)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
|
@J-Son89 e2e are okay. Ready for design review cc @Francesca-G |
Francesca-G
approved these changes
Oct 11, 2023
There was a problem hiding this comment.
Looks good ✨
@pavloburykh I saw you already noticed the light blur issue, so I didn't leave any comment on that. A quick update about the copy of the button overall, as clarified here, we should always use "Slide" instead of sign (which was featured in some wallet designs).
Examples:
- Slide to reveal code
- Slide to create account
- Slide to reveal phrase
- Slide to bridge (previously "Slide to sign")
BalogunofAfrica
approved these changes
Oct 11, 2023
J-Son89
force-pushed
the
jc/slider-button
branch
from
735a319 to
f224118
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.
fixes: #17444
This pr adds the blur variant for the slider button.
It also adds the blur. Note - with the blur & error variant - I have not implemented the changing background color yet as that will involve some further in put from design on what is expected of the color transition. Something I only discovered while implementing these changes. Will create a follow up for that.
blur
blur and type danger
Syncing Page:
To Test:
only changes to slider button so wherever this is used in the app - currently only in the syncing screens (afaik)
design review:
check the slider button in quo2 preview