[#18083] Implement Page top component #18163

ulisesmac · 2023-12-13T02:24:50Z

Summary

This PR implements the page top component:

https://www.figma.com/file/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile?type=design&node-id=8737-151877&mode=dev

Review notes

There's a component called text-combinations (quo.components.text-combinations.view) in the app, but it's not listed in figma and it has many usages across the onboarding and wallet screens, this new added component will substitute it since this is a full implementation of the figma designs (and the existing one is incomplete, for example, it doesn't contemplate inputs and context tags).

I'm planning to refactor the wallet and onboarding screens using this component in a later issue.

Platforms

Android
iOS

Steps to test

Open Status
Navigate to quo 2 library -> text combinations -> page-top

status: ready

ulisesmac · 2023-12-13T02:30:05Z

src/quo/components/inputs/address_input/view.cljs

-           :auto-complete          (when platform/ios? :none)
+           :auto-complete          (when platform/ios? :off)


Since the address-input is being used by the page-top component, I had to fix this, it throws a warning that breaks the tests

status-im-auto · 2023-12-13T02:30:57Z

Jenkins Builds

Click to see older builds (5)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	`99235fd`	#1	2023-12-13 02:30:56	~5 min	`tests`	📄`log`
✔️	`99235fd`	#1	2023-12-13 02:34:05	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`99235fd`	#1	2023-12-13 02:34:14	~9 min	`android`	🤖`apk` 📲
✔️	`99235fd`	#1	2023-12-13 02:38:20	~13 min	`ios`	📱`ipa` 📲
✔️	`99235fd`	#2	2023-12-13 02:42:23	~2 min	`tests`	📄`log`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`8efa05a`	#3	2023-12-13 02:50:08	~4 min	`tests`	📄`log`
✔️	`8efa05a`	#2	2023-12-13 02:52:15	~6 min	`android`	🤖`apk` 📲
✔️	`8efa05a`	#2	2023-12-13 02:53:08	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`8efa05a`	#2	2023-12-13 02:58:52	~13 min	`ios`	📱`ipa` 📲

✔️	`d7ce6a9`	#4	2023-12-20 19:13:37	~4 min	`tests`	📄`log`
✔️	`d7ce6a9`	#3	2023-12-20 19:14:39	~5 min	`ios`	📱`ipa` 📲
✔️	`d7ce6a9`	#3	2023-12-20 19:15:36	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`d7ce6a9`	#3	2023-12-20 19:16:52	~7 min	`android`	🤖`apk` 📲

OmarBasem · 2023-12-13T05:11:16Z

src/quo/components/text_combinations/page_top/style.cljs

+   :margin-horizontal 20})
+
+(def recovery-phrase-container
+  {:padding-vertical nil


What is :padding-vertical nil for?

It's overriding the original recovery-phrase styles.

That component is setting padding-vertical, in this PR we need the styles to be a different padding-top and padding-bottom, so just to make sure there isn't a padding vertical being passed.

OmarBasem · 2023-12-13T05:13:20Z

src/quo/components/text_combinations/page_top/view.cljs

+      (str "0" num)
+      (str num))))
+
+(defn- header-counter


What is the purpose of this counter?

Match the component styles:

that 00/00

OmarBasem · 2023-12-13T05:16:15Z

The text-combinations is used in many places. Have you checked that everything looks fine?

flexsurfer · 2023-12-13T08:59:12Z

it's not always we should treat figma components as quo components, imo, this one is the case, it doesn't look like a component, it's just a different combinations of different components that can be done in each view

ulisesmac · 2023-12-13T15:50:29Z

The text-combinations is used in many places. Have you checked that everything looks fine?

@OmarBasem
I'll ask for a design review in this PR.

And still not replacing the usages in this PR, as mentioned in the description, I'm planning to do it in a later one 👍

ulisesmac · 2023-12-13T15:59:13Z

it's not always we should treat figma components as quo components, imo, this one is the case, it doesn't look like a component, it's just a different combinations of different components that can be done in each view

@flexsurfer

I agree. While creating it, I thought the same: it could be done on each screen.

But when we create page-top as a component and use it, we make sure margin, paddings, sizes, etc, are correct. IMO, that is easier than replicating the same composition of components on each page.

Currently, text-combinations is the one used in many screens (onboarding and wallet namespaces), and page-top is just the complete implementation of if.

vkjr · 2023-12-19T15:14:21Z

src/quo/components/text_combinations/page_top/component_spec.cljs

+   :community-name      "Coinbase"
+   :emoji               "😝"})
+
+(h/describe "Page Top"


Could you please also add the test rendering component without any props?

Hi @vkjr
I'm wondering why do we want to sucessfully render the component without props?

mmilad75

LGTM!

ulisesmac · 2023-12-19T23:30:50Z

Hi @Francesca-G :)
I'll need a design review for this component

Francesca-G

Nice work ✨

ulisesmac · 2023-12-20T19:17:59Z

Merged without QA because it's a new component not used yet in screens

ulisesmac requested review from vkjr, tumanov-alex, briansztamfater, smohamedjavid, J-Son89, OmarBasem and mmilad75 December 13, 2023 02:24

ulisesmac self-assigned this Dec 13, 2023

ulisesmac commented Dec 13, 2023

View reviewed changes

ulisesmac force-pushed the 18083-page-top-component branch from 99235fd to 8efa05a Compare December 13, 2023 02:45

OmarBasem reviewed Dec 13, 2023

View reviewed changes

J-Son89 requested a review from shivekkhurana December 14, 2023 16:42

OmarBasem approved these changes Dec 19, 2023

View reviewed changes

vkjr reviewed Dec 19, 2023

View reviewed changes

vkjr approved these changes Dec 19, 2023

View reviewed changes

mmilad75 approved these changes Dec 19, 2023

View reviewed changes

Francesca-G approved these changes Dec 20, 2023

View reviewed changes

ulisesmac added 4 commits December 20, 2023 12:50

Add container style and fix :auto-complete parameter in address-input

e4feb29

Add container style to recovery-phrase input

02c8e9f

Add container style and accessibility-label to search-input

7794b32

Add page-top component and tests

b065d5f

Add page top preview

d7ce6a9

ulisesmac force-pushed the 18083-page-top-component branch from 8efa05a to d7ce6a9 Compare December 20, 2023 19:08

ulisesmac merged commit 7cf3165 into develop Dec 20, 2023
6 checks passed

ulisesmac deleted the 18083-page-top-component branch December 20, 2023 19:17

ilmotta mentioned this pull request Dec 26, 2023

Use text-combinations/page-top component in request to join sheet. #18125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#18083] Implement Page top component #18163

[#18083] Implement Page top component #18163

ulisesmac commented Dec 13, 2023 •

edited

Loading

ulisesmac Dec 13, 2023

status-im-auto commented Dec 13, 2023 •

edited

Loading

OmarBasem Dec 13, 2023

ulisesmac Dec 19, 2023

OmarBasem Dec 13, 2023

ulisesmac Dec 19, 2023

OmarBasem commented Dec 13, 2023

flexsurfer commented Dec 13, 2023 •

edited

Loading

ulisesmac commented Dec 13, 2023

ulisesmac commented Dec 13, 2023

vkjr Dec 19, 2023

ulisesmac Dec 19, 2023

mmilad75 left a comment

ulisesmac commented Dec 19, 2023

Francesca-G left a comment

ulisesmac commented Dec 20, 2023

		:auto-complete (when platform/ios? :none)
		:auto-complete (when platform/ios? :off)

[#18083] Implement Page top component #18163

[#18083] Implement Page top component #18163

Conversation

ulisesmac commented Dec 13, 2023 • edited Loading

Summary

Review notes

Platforms

Steps to test

ulisesmac Dec 13, 2023

Choose a reason for hiding this comment

status-im-auto commented Dec 13, 2023 • edited Loading

Jenkins Builds

OmarBasem Dec 13, 2023

Choose a reason for hiding this comment

ulisesmac Dec 19, 2023

Choose a reason for hiding this comment

OmarBasem Dec 13, 2023

Choose a reason for hiding this comment

ulisesmac Dec 19, 2023

Choose a reason for hiding this comment

OmarBasem commented Dec 13, 2023

flexsurfer commented Dec 13, 2023 • edited Loading

ulisesmac commented Dec 13, 2023

ulisesmac commented Dec 13, 2023

vkjr Dec 19, 2023

Choose a reason for hiding this comment

ulisesmac Dec 19, 2023

Choose a reason for hiding this comment

mmilad75 left a comment

Choose a reason for hiding this comment

ulisesmac commented Dec 19, 2023

Francesca-G left a comment

Choose a reason for hiding this comment

ulisesmac commented Dec 20, 2023

ulisesmac commented Dec 13, 2023 •

edited

Loading

status-im-auto commented Dec 13, 2023 •

edited

Loading

flexsurfer commented Dec 13, 2023 •

edited

Loading