feat: browser input #16039

BalogunofAfrica · 2023-05-29T09:01:40Z

Fixes #15972

Implements the browser input component according to the designs.

The component is added to the quo preview in the profile settings.

status: ready

status-github-bot · 2023-05-29T09:01:45Z

Hey @BalogunofAfrica, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2

status-im-auto · 2023-05-29T09:08:12Z

Jenkins Builds

Click to see older builds (16)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`07e6fd1`	#1	2023-05-29 09:08:12	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`07e6fd1`	#1	2023-05-29 09:08:35	~6 min	`android`	🤖`apk` 📲
✔️	`07e6fd1`	#1	2023-05-29 09:10:42	~8 min	`tests`	📄`log`
✔️	`07e6fd1`	#1	2023-05-29 09:11:41	~9 min	`ios`	📱`ipa` 📲

❌	`e526d62`	#2	2023-05-29 09:50:18	~3 min	`tests`	📄`log`
✔️	`e526d62`	#2	2023-05-29 09:52:35	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`e526d62`	#2	2023-05-29 09:53:16	~6 min	`ios`	📱`ipa` 📲
✔️	`e526d62`	#2	2023-05-29 09:54:28	~8 min	`android`	🤖`apk` 📲

❌	`44c5e70`	#3	2023-06-01 23:27:57	~4 min	`tests`	📄`log`
✔️	`44c5e70`	#3	2023-06-01 23:29:39	~5 min	`android`	🤖`apk` 📲
✔️	`44c5e70`	#3	2023-06-01 23:29:49	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`44c5e70`	#3	2023-06-01 23:30:42	~6 min	`ios`	📱`ipa` 📲

✔️	`e8dac42`	#4	2023-06-02 10:53:11	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`e8dac42`	#4	2023-06-02 10:53:33	~6 min	`android`	🤖`apk` 📲
✔️	`e8dac42`	#4	2023-06-02 10:53:53	~6 min	`ios`	📱`ipa` 📲
✔️	`e8dac42`	#4	2023-06-02 10:57:48	~10 min	`tests`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`4721034`	#5	2023-06-02 12:35:17	~5 min	`android-e2e`	🤖`apk` 📲
✔️	`4721034`	#5	2023-06-02 12:35:59	~5 min	`android`	🤖`apk` 📲
✔️	`4721034`	#5	2023-06-02 12:36:37	~6 min	`ios`	📱`ipa` 📲
✔️	`4721034`	#5	2023-06-02 12:39:43	~9 min	`tests`	📄`log`

✔️	`74489bb`	#6	2023-06-05 11:28:10	~5 min	`android`	🤖`apk` 📲
✔️	`74489bb`	#6	2023-06-05 11:28:55	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`74489bb`	#6	2023-06-05 11:29:34	~6 min	`ios`	📱`ipa` 📲
✔️	`74489bb`	#6	2023-06-05 11:31:33	~8 min	`tests`	📄`log`

src/quo2/components/inputs/browser_input/style.cljs

src/quo2/components/inputs/browser_input/view.cljs

briansztamfater · 2023-05-29T09:23:38Z

Hi @BalogunofAfrica thanks for your PR! Left some minor comments but overall looks good.
Also would be good to have some screenshots on the PR description so we can get see a preview of the results.

ilmotta

LGTM, great work and thanks @BalogunofAfrica

BalogunofAfrica · 2023-05-29T10:11:03Z

@briansztamfater Thanks for the feedback!

I've updated the PR with the requested changes and also included a screenshot.

siddarthkay

nice job.

flexsurfer

thank you for the contribution, left a few comments

flexsurfer · 2023-06-01T08:04:16Z

src/quo2/components/inputs/browser_input/view.cljs

+        set-active  #(reset! state :active)
+        set-default #(reset! state :default)
+        use-value?  (boolean value)]
+    (fn [{:keys [value disabled? blur? on-change-text customization-color


value shouldn't be used , default-value should be used instead

@flexsurfer the value prop is not used by default but can be used when we want to implement clearing the input

Oh, seen the implementation of text-input would change it to an uncontrolled input with ref to clear input when needed

still we shouldn't manage value outside, it should be default-value, and value should be used only for clear-button button but it should be managed by component itself

still we shouldn't manage value outside, it should be default-value, and value should be used only for clear-button button but it should be managed by component itself

That is from the previous commit, I have removed it in the latest push I made to this branch.

See here

i mean there should be no value in params

Oh I get you, would make that fix now

@flexsurfer Updated the PR, moved value and the clear functionality into the component.

flexsurfer · 2023-06-01T08:05:22Z

src/quo2/components/inputs/browser_input/view.cljs

+                 on-clear on-focus on-blur override-theme]
+          :or   {customization-color :blue}
+          :as   props}]
+      (let [clean-props (apply dissoc props props-to-remove)]


could you please elaborate on clean-props looks strange why don't just merge them ?

The clean-props consist of the props that have not been applied earlier to the text input, merging them would have overwritten the props we have used for the text input. See from line 64 as the beginning of where we pass some of these props to the text input

flexsurfer · 2023-06-01T08:06:13Z

src/quo2/components/inputs/browser_input/style.cljs

+
+(defn input-container
+  [disabled?]
+  (assoc (text/text-style {:size   :paragraph-1


hey @ulisesmac how is this implemented in text-input? also with text/text-style usage?

@flexsurfer Yes, also calling this function to get the styles

flexsurfer · 2023-06-01T08:07:52Z

src/quo2/components/inputs/browser_input/view.cljs

+                   :on-change-text         (fn [new-text]
+                                             (when on-change-text
+                                               (on-change-text new-text))
+                                             (reagent/flush))


could you pls elaborate on reagent/flush usage

It was added to trigger an instant update of the UI with the on-change event.

I have updated the PR and removed it, it would no longer be needed as the component is an uncontrolled input now.

cammellos

Hi @BalogunofAfrica , thanks for the PR.

One thing I could not see in the preview, is the state where the url is not being edited (not in focus).

With the favicon on the left (if available), and the padlock icon on the right (if ssl is used, so it should be toggled on/off), maybe I am mistaken, is that implemented (first entry in the design on figma)?

Thanks!

BalogunofAfrica · 2023-06-01T23:56:32Z

@flexsurfer @cammellos Thanks for the feedback!

I have updated the PR to address every point stated and also replied to the reviews.

I have also updated the screenshot

flexsurfer · 2023-06-02T07:56:03Z

src/quo2/components/inputs/browser_input/view.cljs

+                    :selection-color        (when platform/ios?
+                                              (cursor-color customization-color override-theme))
+                    :style                  (style/input disabled?)}
+             (seq clean-props) (merge clean-props))]


i don't think complexity with clean-props is needed, we could just merge and override with our map

flexsurfer · 2023-06-02T07:57:00Z

src/quo2/components/inputs/browser_input/view.cljs

+              [lock-icon
+               {:blur?          blur?
+                :override-theme override-theme
+               }])])


:override-theme override-theme}])])

flexsurfer · 2023-06-02T08:01:09Z

src/quo2/components/inputs/browser_input/view.cljs

+        set-active  #(reset! state :active)
+        set-default #(reset! state :default)
+        use-value?  (boolean value)]
+    (fn [{:keys [value disabled? blur? on-change-text customization-color


still we shouldn't manage value outside, it should be default-value, and value should be used only for clear-button button but it should be managed by component itself

cammellos · 2023-06-05T11:27:18Z

Hi @BalogunofAfrica , I just spoke to @pedro-et , our lead designer, to clarify some interactions.

I have noticed that the bar/input are two separate components, but they should be one.

The effect we are looking for is similar to how Safari does it, I have attached a video to better show it

RPReplay_Final1685953709.mov

Basically, once you focus the input, it should make it editable and with the text fully selected, let me know if you have any questions, and thank you for your work!

BalogunofAfrica · 2023-06-05T11:38:31Z

@cammellos

Oh, I wasn't aware of that requirement. I would look into it.

BalogunofAfrica · 2023-06-06T15:40:42Z

@cammellos I would like to confirm if this requirement is for iOS alone or both platforms.

I ask because this particular interaction/feature of attaching input with the soft keyboard is peculiar to iOS, and not so on Android.

cc: @pedro-et

flexsurfer · 2023-06-15T08:01:40Z

hey @BalogunofAfrica sorry for the late response, this requirement is for both platforms.

flexsurfer · 2023-06-23T08:15:14Z

feel free to reopen

BalogunofAfrica · 2023-07-04T22:36:24Z

@flexsurfer Re-opened in #16487

briansztamfater reviewed May 29, 2023

View reviewed changes

src/quo2/components/inputs/browser_input/style.cljs Outdated Show resolved Hide resolved

briansztamfater reviewed May 29, 2023

View reviewed changes

src/quo2/components/inputs/browser_input/style.cljs Outdated Show resolved Hide resolved

briansztamfater reviewed May 29, 2023

View reviewed changes

src/quo2/components/inputs/browser_input/style.cljs Outdated Show resolved Hide resolved

briansztamfater reviewed May 29, 2023

View reviewed changes

src/quo2/components/inputs/browser_input/view.cljs Outdated Show resolved Hide resolved

briansztamfater reviewed May 29, 2023

View reviewed changes

src/quo2/components/inputs/browser_input/view.cljs Outdated Show resolved Hide resolved

briansztamfater added the external label May 29, 2023

briansztamfater assigned BalogunofAfrica May 29, 2023

siddarthkay requested review from ilmotta, Parveshdhull, J-Son89 and siddarthkay May 29, 2023 09:27

BalogunofAfrica force-pushed the feat/browser-input branch from 07e6fd1 to e526d62 Compare May 29, 2023 09:46

ilmotta approved these changes May 29, 2023

View reviewed changes

BalogunofAfrica requested a review from briansztamfater May 30, 2023 15:06

siddarthkay approved these changes May 30, 2023

View reviewed changes

flexsurfer requested changes Jun 1, 2023

View reviewed changes

cammellos suggested changes Jun 1, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

BalogunofAfrica requested review from flexsurfer and cammellos June 2, 2023 06:55

flexsurfer reviewed Jun 2, 2023

View reviewed changes

BalogunofAfrica force-pushed the feat/browser-input branch from 44c5e70 to e8dac42 Compare June 2, 2023 10:46

BalogunofAfrica requested a review from flexsurfer June 2, 2023 10:47

BalogunofAfrica force-pushed the feat/browser-input branch from e8dac42 to 4721034 Compare June 2, 2023 12:29

BalogunofAfrica requested a review from ulisesmac June 2, 2023 12:33

feat: add browser input component to quo2

74489bb

BalogunofAfrica force-pushed the feat/browser-input branch from 4721034 to 74489bb Compare June 5, 2023 11:22

flexsurfer closed this Jun 23, 2023

BalogunofAfrica mentioned this pull request Jul 4, 2023

feat: browser input #16487

Merged

feat: browser input #16039

feat: browser input #16039

Conversation

BalogunofAfrica commented May 29, 2023 • edited Loading

status-github-bot bot commented May 29, 2023

status-im-auto commented May 29, 2023 • edited Loading

Jenkins Builds

briansztamfater commented May 29, 2023

ilmotta left a comment

Choose a reason for hiding this comment

BalogunofAfrica commented May 29, 2023 • edited Loading

siddarthkay left a comment

Choose a reason for hiding this comment

flexsurfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cammellos left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

BalogunofAfrica commented Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cammellos commented Jun 5, 2023

BalogunofAfrica commented Jun 5, 2023 • edited Loading

BalogunofAfrica commented Jun 6, 2023 • edited Loading

flexsurfer commented Jun 15, 2023

flexsurfer commented Jun 23, 2023

BalogunofAfrica commented Jul 4, 2023

BalogunofAfrica commented May 29, 2023 •

edited

Loading

status-im-auto commented May 29, 2023 •

edited

Loading

BalogunofAfrica commented May 29, 2023 •

edited

Loading

BalogunofAfrica commented Jun 1, 2023 •

edited

Loading

BalogunofAfrica commented Jun 5, 2023 •

edited

Loading

BalogunofAfrica commented Jun 6, 2023 •

edited

Loading