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.

Sign up for GitHub

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

Jump to bottom

feat: browser input #16487

Merged
merged 5 commits into from
Jul 24, 2023
Merged

feat: browser input #16487

merged 5 commits into from
Jul 24, 2023

Conversation

BalogunofAfrica
Copy link
Contributor

@BalogunofAfrica BalogunofAfrica commented Jul 4, 2023 •
edited
Loading

Reopens #16039
Fixes #15972

Implements the browser input component according to the designs.

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

Preview 1 Preview 2 Preview 3
Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-07-06.at.19.55.48.mp4

Component overlayed with the design:

Screenshot 2023-07-04 at 22 59 12

status: ready

@BalogunofAfrica BalogunofAfrica mentioned this pull request Jul 4, 2023
Closed
@BalogunofAfrica BalogunofAfrica force-pushed the feat/browser-input branch 2 times, most recently from 5a07e9f to 8c60126 Compare July 6, 2023 16:37
ibrkhalil
ibrkhalil reviewed Jul 6, 2023
View reviewed changes
Copy link
Contributor

@ibrkhalil ibrkhalil left a comment

Choose a reason for hiding this comment

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

Should the browser default to a search engine like Google, Yahoo, Bing in case the user didn't add a specifc URL?
And should we allow him to alter the selection of the default engine?

src/quo2/components/browser/browser_input/style.cljs Outdated
:color
(colors/theme-colors colors/neutral-100 colors/white)))

(def rooot-container
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, should be root-container

src/quo2/components/browser/browser_input/view.cljs Outdated
:style style/clear-icon-container}
[icon/icon :i/clear
{:color (clear-icon-color blur? theme)
:size 20}]])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think default size is 20 so this can be omitted, By no worries let's keep it.

Copy link
Contributor

@J-Son89 J-Son89 Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

let's remove it if it's redundant 👍

@BalogunofAfrica
Copy link
Contributor Author

BalogunofAfrica commented Jul 7, 2023 •
edited
Loading

Should the browser default to a search engine like Google, Yahoo, Bing in case the user didn't add a specifc URL? And should we allow him to alter the selection of the default engine?

This PR only adds the input component for the browser and doesn't implement the actual search/query which is where I think the consideration would come in.

@status-im-auto
Copy link
Member

status-im-auto commented Jul 17, 2023 •
edited
Loading

Jenkins Builds

Click to see older builds (12)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
❌ 0a7c78a #1 2023-07-17 18:41:21 ~3 min tests 📄log
✔️ 0a7c78a #1 2023-07-17 18:44:27 ~7 min ios 📱ipa 📲
✔️ 0a7c78a #1 2023-07-17 18:45:18 ~7 min android 🤖apk 📲
✔️ 0a7c78a #1 2023-07-17 18:45:53 ~8 min android-e2e 🤖apk 📲
✔️ 9323b56 #2 2023-07-18 09:28:54 ~5 min android-e2e 🤖apk 📲
✔️ 9323b56 #2 2023-07-18 09:29:05 ~5 min android 🤖apk 📲
✔️ 9323b56 #2 2023-07-18 09:29:43 ~6 min ios 📱ipa 📲
❌ 9323b56 #2 2023-07-18 09:31:30 ~8 min tests 📄log
✔️ b671875 #3 2023-07-18 15:16:41 ~6 min android-e2e 🤖apk 📲
✔️ b671875 #3 2023-07-18 15:18:32 ~8 min android 🤖apk 📲
✔️ b671875 #3 2023-07-18 15:18:42 ~8 min ios 📱ipa 📲
❌ b671875 #3 2023-07-18 15:18:49 ~8 min tests 📄log
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6bb6704 #6 2023-07-18 16:01:02 ~5 min android 🤖apk 📲
✔️ 6bb6704 #6 2023-07-18 16:01:24 ~6 min android-e2e 🤖apk 📲
✔️ 6bb6704 #6 2023-07-18 16:03:02 ~7 min tests 📄log
✔️ 6bb6704 #6 2023-07-18 16:07:02 ~11 min ios 📱ipa 📲
✔️ 0186c3e #7 2023-07-24 11:31:44 ~5 min android-e2e 🤖apk 📲
✔️ 0186c3e #7 2023-07-24 11:33:48 ~7 min android 🤖apk 📲
✔️ 0186c3e #7 2023-07-24 11:33:49 ~7 min ios 📱ipa 📲
✔️ 0186c3e #7 2023-07-24 11:34:58 ~8 min tests 📄log

@cammellos
Copy link
Contributor

@BalogunofAfrica linting fails:


Configuring Nix shell for target 'clojure'...

bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)

bash: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)

sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)

Node modules not created by Nix

Copying node_modules from Nix store:

 - /nix/store/asi99lnx45c0kdpmxcg5xmplwflnp12i-status-mobile-node-deps-1.23.0-patched

Done in: 2.052s

sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)

src/status_im2/contexts/quo_preview/browser/browser_input.cljs:24:22: warning: unused binding keyboard-show-listener

src/status_im2/contexts/quo_preview/browser/browser_input.cljs:27:22: warning: unused binding keyboard-hide-listener

linting took 8590ms, errors: 0, warnings: 2

make: *** [Makefile:306: lint] Error 2

script returned exit code 2

You might want to address the issues and run make lint afterward to make sure it's ok.
Thank you!

@BalogunofAfrica
Copy link
Contributor Author

@cammellos I have updated the PR with the fix

@cammellos
Copy link
Contributor

@BalogunofAfrica thank you, lint passes!

Component tests are now failing:

PASS component-spec/quo2.components.drawers.permission_context.component_spec.js

FAIL component-spec/quo2.components.browser.browser_input.component_spec.js (197.484 s)

  ● Browser input › Input Label › Renders label text when specified



    expect(received).toBeTruthy()



    Received: null



      at test-helpers.component/query-by-label-text (test_helpers/component.cljs:189:27)

      at Object.<anonymous> (quo2/components/browser/browser_input/component_spec.cljs:28:21)



  ● Browser input › Input Label › Renders label favicon when specified alongside label text



    expect(received).toBeTruthy()



    Received: null



      at test-helpers.component/query-by-label-text (test_helpers/component.cljs:189:27)

      at Object.<anonymous> (quo2/components/browser/browser_input/component_spec.cljs:33:21)



  ● Browser input › Input Label › Renders lock icon when using ssl alongside label text



    expect(received).toBeTruthy()



    Received: null



      at test-helpers.component/query-by-label-text (test_helpers/component.cljs:189:27)

      at Object.<anonymous> (quo2/components/browser/browser_input/component_spec.cljs:38:21)

would you mind having a look please?

thank you

@BalogunofAfrica BalogunofAfrica force-pushed the feat/browser-input branch 2 times, most recently from 7c25303 to 9323b56 Compare July 18, 2023 15:53
@BalogunofAfrica
Copy link
Contributor Author

@cammellos Fixed test errors

@cammellos
Copy link
Contributor

Thank you @BalogunofAfrica , amazing work!

flexsurfer
flexsurfer approved these changes Jul 24, 2023
View reviewed changes
src/quo2/components/browser/browser_input/style.cljs Outdated
:flex-direction :row
:justify-content :center
:opacity (if display? 1 0)
:width "100%"})
Copy link
Member

Choose a reason for hiding this comment

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

width is redundant

src/quo2/components/browser/browser_input/style.cljs Outdated
:height 20
:justify-content :center
:margin-left 8
:top 3.5
Copy link
Member

Choose a reason for hiding this comment

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

should be integer

src/quo2/components/browser/browser_input/view.cljs Outdated
(when on-clear (on-clear)))
:theme theme}])]]))))


Copy link
Member

Choose a reason for hiding this comment

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

extra line

src/quo2/components/browser/browser_input/view.cljs
on-clear on-focus on-blur theme get-ref locked?
favicon favicon-color favicon-size]
:as props}]
(let [clean-props (apply dissoc props props-to-remove)]
Copy link
Member

Choose a reason for hiding this comment

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

could you pls elaborate on why we need to remove props?

src/quo2/components/browser/browser_input/view.cljs
[react-native.core :as rn]
[reagent.core :as reagent]
[quo2.foundations.colors :as colors]
[quo2.theme :as quo2.theme]
Copy link
Member

Choose a reason for hiding this comment

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

just theme, quo2 is redundant

Copy link
Contributor

Choose a reason for hiding this comment

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

actually it's not @flexsurfer. Here we are using theme prop as well so there is a likely chance these two references clash and there is unsafe uses.
Similar to @ilmotta's pr: #16500

however we have been using [quo2.theme :as quo.theme] in other files

src/quo2/components/browser/browser_input/view.cljs Outdated
(when (and (seq @value) (= @state :default))
[rn/touchable-opacity
{:style style/default-container
:on-press (fn [] (focus-input))}
Copy link
Member

Choose a reason for hiding this comment

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

fn [] is redundant just focus-input

@flexsurfer flexsurfer merged commit c5a4866 into status-im:develop Jul 24, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J-Son89 J-Son89 J-Son89 approved these changes

@Francesca-G Francesca-G Francesca-G approved these changes

@flexsurfer flexsurfer flexsurfer approved these changes

@cammellos cammellos Awaiting requested review from cammellos

@rahulpsingh rahulpsingh Awaiting requested review from rahulpsingh

@ibrkhalil ibrkhalil Awaiting requested review from ibrkhalil

Assignees
No one assigned
Labels
external
Projects
No open projects
Legacy QA Automation Project
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Browser input - Quo2 Component
7 participants
@BalogunofAfrica @status-im-auto @cammellos @flexsurfer @J-Son89 @ibrkhalil @Francesca-G