chore: add floating button page component #17737
Merged
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
Jenkins BuildsClick to see older builds (81)
|
J-Son89
force-pushed
the
jc/floating-button-page
branch
3 times, most recently
from
4c20718
to
1080aee
Compare
J-Son89
commented
Oct 30, 2023
src/status_im2/common/floating_button_page/floating_container/style.cljs
Outdated
Show resolved
Hide resolved
J-Son89
commented
Oct 30, 2023
src/status_im2/common/floating_button_page/floating_container/style.cljs
Outdated
Show resolved
Hide resolved
J-Son89
commented
Oct 30, 2023
J-Son89
commented
Oct 30, 2023
J-Son89
commented
Oct 30, 2023
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
J-Son89
force-pushed
the
jc/floating-button-page
branch
2 times, most recently
from
8ed0a6a
to
f21d49f
Compare
J-Son89
requested review from
ilmotta,
ulisesmac,
briansztamfater,
jo-mut,
OmarBasem and
smohamedjavid
OmarBasem
reviewed
Oct 31, 2023
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
J-Son89
force-pushed
the
jc/floating-button-page
branch
from
f21d49f
to
ee860aa
Compare
| @@ -66,7 +66,7 @@ | |||
| :background :blur | |||
| :on-press #(rf/dispatch [:navigate-back]) | |||
| :right-side [{:icon-left :i/info | |||
| :label (i18n/label :t/how-to-pair) | |||
| :label "swap button" | |||
There was a problem hiding this comment.
I guess you need to revert the changes here :)
There was a problem hiding this comment.
@ulisesmac let's revert
| (:require | ||
| [quo.foundations.colors :as colors])) | ||
|
|
||
| (defn main |
There was a problem hiding this comment.
As there are no arguments received by this function, we can convert this to def.
There was a problem hiding this comment.
Here we need it as a function because when theme-colors doesn't receive the override-theme arg it fallback to (theme/get-theme).
| :value ""}] | ||
| [quo/button | ||
| {:type :outline | ||
| :on-press #(swap! content-height (fn [v] (+ v 10)))} |
There was a problem hiding this comment.
partial is your friend here.
Suggested change
| :on-press #(swap! content-height (fn [v] (+ v 10)))} | |
| :on-press #(swap! content-height (partial + 10))} |
There was a problem hiding this comment.
Following up on @smohamedjavid suggestion, we don't need an anonymous function.
(let [a (atom 1)]
(swap! a + 10)
@a) ; => 11So just #(swap! content-height + 10) is enough. Same idea for the on-press that subtracts 10 a few lines below.
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im2/common/floating_button_page/floating_container/view.cljs
Outdated
Show resolved
Hide resolved
ulisesmac
force-pushed
the
jc/floating-button-page
branch
from
ee860aa
to
f3c8413
Compare
|
@J-Son89 I am curious on the Status IM components. As you mentioned there is more work around this to be done. Are these components basically the ones on common namespace within statusi-im2? |
ulisesmac
force-pushed
the
jc/floating-button-page
branch
from
64f6f18
to
6839ffd
Compare
|
Hey @jo-mut @smohamedjavid @OmarBasem @J-Son89 and I are solving some issues, I'll let you know when to continue with the review. Thanks! :) |
pretty much, these components can live anywhere in status-im2 ns. Basically some functionality which is in the designs but not abstracted to a design component, or we have realised the pattern ourselves and made an abstraction. However often it is difficult to develop these properly if we're working on a specific page so the preview page is an easier way to isolate these components and have a place to develop them properly where the specifics can be tested, verified and maintained 👌 |
Now I understand. I think this is very useful and its definately something we should implement. Thank you. |
ulisesmac
force-pushed
the
jc/floating-button-page
branch
from
016f4d1
to
b0b3ff2
Compare
82% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (37)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Merged
|
Hi @J-Son89 thank you for PR. Should in the scope of this PR be applied the current button showing on the pages described in issue 1? ISSUE 1: The button is not shown above the keyboardSteps:
Actual result:The button is not shown above the keyboard on the following pages:
Expected result:Button should be displayed above the keyboard when keyboard is enabled |
ISSUE 2: [Android] The button is not fully shown on the 'Add address to watch' pageSteps:
Actual result:The button is slightly hidden on 'Add address to watch' screen when keyboard is enabled/disabled Expected result:The button is fully shown on 'Add address to watch' screen Last develop Devices:
|
|
oh this pr just adds the component to the codebase, we haven't added it to the screens yet. Those ui issues on wallet are separate to this pr and the same as develop 👍 To check the component it is in profile -> STAtus IM Components -> Common -> Floating Button Page however I think in terms of testing this pr just needs a smoke check to make sure it is not breaking the app. |
@J-Son89 Thank you for clarification. No other issues from my side. PR is ready to be merged |
J-Son89
force-pushed
the
jc/floating-button-page
branch
3 times, most recently
from
5058804
to
3072e8c
Compare
J-Son89
force-pushed
the
jc/floating-button-page
branch
from
3072e8c
to
da09c54
Compare
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
Co-authored-by: Ulises M <ulises.ssb@hotmail.com>
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: #17747
This pr adds a common page component that is consistently used in the designs. It is for pages that have scrolling content and a button at the bottom to call some action. when the keyboard is visible on these pages the button should sit on top of the keyboard and if the button is over the content of the page then a blur background will sit behind the button. The button can be a slide button or a regular button.
Additionally this pr starts adding a preview space for Status IM common components to be displayed. More work around this will be created, for now I added the minimum required to get this started.
A video of it working on ios
Screen.Recording.2023-10-30.at.22.55.56.mov
Checked with Figma and everything is aligning nicely 👍
No test needed as this is only a new component not touching production code
Design review will be nice to verify the behaviour is working as expected, however it will be easier to check on a nicer page then the preview screen I created :)