Implement Quo2 Numbered-Keyboard / Keyboard Key component #16723
Conversation
Jenkins BuildsClick to see older builds (40)
|
ajayesivan
requested review from
J-Son89,
rahulpsingh,
smohamedjavid and
ajayesivan
src/quo2/core.cljs
Outdated
| @@ -188,6 +189,9 @@ | |||
| (def search-input quo2.components.inputs.search-input.view/search-input) | |||
| (def title-input quo2.components.inputs.title-input.view/title-input) | |||
|
|
|||
| ;;;; Keyboard | |||
There was a problem hiding this comment.
nit: this should be upper case for consistency and also matching the area name, i.e
;;;; NUMBERED KEYBOARD
There was a problem hiding this comment.
Just jumping in to share my perspective.
I wish we didn't follow this convention. It's also not in the Clojure Style Guide https://guide.clojure.style/#four-semicolons-for-heading-comments
For an Emacs user, heading comments can be easily syntax highlighted (even by level). Vim should probably have this capability but I'm not using it anymore.
But I totally get why devs using other editors opted for using the screaming case to differentiate between normal comments and heading comments :)
There was a problem hiding this comment.
tbh I don't think there's much of a convention there, we can just change that file to use 4 afaik 👍
There was a problem hiding this comment.
@J-Son89, I meant the case of the text is all uppercased. I know this is not important, who cares about the case of the heading right? But anyway it's not in the Clojure style guide.
There was a problem hiding this comment.
Ah apologies, yeah I think we can just update this so?
There was a problem hiding this comment.
I would say so, our guidelines say we follow the CSG unless stated otherwise.
| @@ -219,6 +220,10 @@ | |||
| {:name :title-input | |||
| :options {:topBar {:visible true}} | |||
| :component title-input/preview-title-input}] | |||
| :keyboard [{:name :keyboard-key | |||
There was a problem hiding this comment.
this key should match figma to make it easy for devs to know where it's corresponding to
i.e :keyboard -> :numbered-keyboard
| (ns quo2.components.numbered-keyboard.keyboard-key.view | ||
| (:require [react-native.core :as rn] | ||
| [quo2.theme :as quo.theme] | ||
| [status-im.ui.components.icons.icons :as icons] |
There was a problem hiding this comment.
| [status-im.ui.components.icons.icons :as icons] | |
| [quo2.components.icon :as icons] |
status-im namespace contain outdated code
There was a problem hiding this comment.
yep, avoid using status-im and quo namespaces where possible :)
| (defn- keyboard-key-internal | ||
| [{:keys [label? disabled theme blur? icon? on-press?]}] | ||
| (let [label-color (get-label-color disabled theme blur?)] | ||
| [rn/touchable-highlight |
There was a problem hiding this comment.
hmm, I think we want to do this the same as the quo2 button component. That is we use the rn/touchable-without-feedback and have an internal state for pressed that we can set the background color on.
| colors/neutral-100))) | ||
|
|
||
| (defn- keyboard-key-internal | ||
| [{:keys [label? disabled theme blur? icon? on-press?]}] |
There was a problem hiding this comment.
Change disabled to disabled?. In Clojure we suffix boolean values with ?.
Is on-press?, label & icon? booleans?
edit: here the only boolean options are disabled and blur so you can change it to
[{:keys [label disabled? theme blur? icon on-press]}]
| colors/neutral-100))) | ||
|
|
||
| (defn- keyboard-key-internal | ||
| [{:keys [label? disabled theme blur? icon? on-press?]}] |
There was a problem hiding this comment.
in clojurescript ? is common practice for marking boolean variables.
| colors/white | ||
| colors/neutral-100))) | ||
|
|
||
| (defn- keyboard-key-internal |
There was a problem hiding this comment.
for labels we have a common pattern as using the second parameter as the label.
i.e
(defn- keyboard-key-internal
[{:keys [ disabled theme blur? icon? on-press?]} label]
I think we should try to stick with Figma here (even if it's a bit much in this case - it will align with best practices then)
e.g
(defn- keyboard-key-internal
[{:keys [disabled theme blur? icon? on-press? type]} label]
(let [label-color (get-label-color disabled theme blur?)]
[rn/touchable-highlight
{:active-opacity 1
:underlay-color (cond
blur? colors/white-opa-10
(= :light theme) colors/neutral-10
(= :dark theme) colors/neutral-80)
:disabled disabled
:on-press on-press?
:style style/container}
(cond
(= type :key) [icons/icon label (style/icon label-color)]
(= type :digit) [rn/text {:style (style/text label-color)} label])]))
| :style style/container} | ||
| (cond | ||
| icon? [icons/icon icon? (style/icon label-color)] | ||
| label? [rn/text {:style (style/text label-color)} label?])])) |
There was a problem hiding this comment.
we want to use quo2 text component here
| (defn text | ||
| [color] | ||
| {:color color | ||
| :font-size 27}) |
There was a problem hiding this comment.
these font sizes are all set by the quo2 text component, in quo2.components.markdown.text
color is already handled there too 👍
| [quo2.foundations.colors :as colors] | ||
| [quo2.components.numbered-keyboard.keyboard-key.style :as style])) | ||
|
|
||
| (defn- get-label-color |
There was a problem hiding this comment.
I think we should put this helper in the styles file 👍
| (defn icon | ||
| [color] | ||
| {:color color | ||
| :width 30 |
There was a problem hiding this comment.
we don't have any icons that are size "30" and in the component it's 20 which is the default
|
let's add a simple component spec test, like a render of each type and an on press 👍 |
| blur? colors/white-opa-10 | ||
| (= :light theme) colors/neutral-10 | ||
| (= :dark theme) colors/neutral-80) | ||
| "transparent")) |
There was a problem hiding this comment.
I think here we should use a keyword to be consistent with the rest of the codebase.
| [{:keys [disabled? theme blur? on-press type]} label] | ||
| (let [label-color (style/get-label-color disabled? theme blur?) | ||
| background-color (style/toggle-background-color @pressed? blur? theme)] | ||
| [rn/touchable-without-feedback |
There was a problem hiding this comment.
I think we should start to use rn/pressable, which I recently added to the namespace react-native.core. The new API is the way to go. It's just as simple :)
There was a problem hiding this comment.
pressable is good, as long as we set the styling to our own designs and not based on the react-native components internal stylings 👍
There was a problem hiding this comment.
If you're looking for a more extensive and future-proof way to handle touch-based input, check out the Pressable API.
@mmilad75, @J-Son89, all RN Touchable* have this yellow warning right at the top. It seems prudent that we define a guideline for using rn/pressable so that we can gradually phase out Touchable* from status-mobile.
cc @OmarBasem, @flexsurfer, @Parveshdhull, @smohamedjavid what do you guys think? Worth of a quick PR to add to the guidelines?
There was a problem hiding this comment.
@ilmotta Yeah I think we should start using Pressable. Although other touchable components are still maintained, I think it is better to start migrating sooner. Also, would unify the touchable components instead of having 3 different components.
There was a problem hiding this comment.
@OmarBasem I replaced it with Pressable
| (defn cool-preview | ||
| [] | ||
| (let [state (reagent/atom {:disabled? false | ||
| :on-press #(println "press") |
There was a problem hiding this comment.
Given the purpose of preview screens is also to help designers review components, I think it's preferable to use js/alert instead of println, or, even better would be to show some kind of UI change once the thing is pressed because who likes alerts right? ;) But a js/alert is what's usually used in preview screens, so it's just fine.
| :align-items :center | ||
| :border-radius 999 | ||
| :background-color color}) | ||
|
|
| [] | ||
| (let [pressed? (reagent/atom false)] | ||
| (fn | ||
| [{:keys [disabled? theme blur? on-press type]} label] |
There was a problem hiding this comment.
[Code style] The arguments' vector should be almost always always in the same line of fn as in (fn [{:keys ...}]).
| :on-press-out #(reset! pressed? false)} | ||
| [rn/view | ||
| {:style (style/container background-color)} | ||
| (cond |
There was a problem hiding this comment.
This is a perfect candidate for condp instead of cond!
| :digit [text/text | ||
| {:weight :regular | ||
| :size :heading-1 | ||
| :style {:color label-color}} label])])))) |
There was a problem hiding this comment.
Quick one: label should be on the next line, otherwise it's hard to read the code. https://guide.clojure.style/#vertically-align-fn-args
|
|
||
| (defn get-label-color | ||
| [disabled? theme blur?] | ||
| (if disabled? |
There was a problem hiding this comment.
consider flattening this structure with a cond, something like
(cond
(and disabled? (or (= :dark theme) blur?)) colors/white-opa-30
(and disabled? (or (= :light theme) blur?)) colors/neutral-30
(or (= :dark theme) blur?) colors/white
:else colors/neutral-100)
|
@Francesca-G components that are about to use in code, but not used yet, are checked by design team. |
|
@churik, @Francesca-G - I asked @mmilad75 to make one small adjustment to the preview screens - it will make reviewing easier imo. Can we wait for @mmilad75 to update with this change and then he can ask @Francesca-G for the review? :) |
|
@Francesca-G Please check it. It's ready for review |
I was about to check this PR but after installing it as usual it just crashes immediately when I try to open it (iPhone 13). Please let me know if it's a problem on my side, I tried to delete the app and re-installing it but it didn't seem to fix the issue RPReplay_Final1690544514.mp4 |
|
@mmilad75 |
src/quo2/core_spec.cljs
Outdated
| @@ -40,4 +40,5 @@ | |||
| [quo2.components.settings.settings-list.component-spec] | |||
| [quo2.components.settings.category.component-spec] | |||
| [quo2.components.share.share-qr-code.component-spec] | |||
| [quo2.components.tags.--tests--.status-tags-component-spec])) | |||
| [quo2.components.tags.--tests--.status-tags-component-spec] | |||
| [quo2.components.numbered-keyboard.keyboard-key.component-spec])) | |||
There was a problem hiding this comment.
actually same here, please keep this alpha sorted 👍 - not all files are alpha sorted but if you can see the pattern is mostly there then it's best to keep it that way and in some cases go one step further and help clean up the ones that are not :)
|
@mmilad75 can you please resolve conflicts and rebase it to the latest develop? |
Done |
Yes, you're right. It was discussed with @J-Son89 and we decided to fix this issue in another ticket since the label I've used is correct, but the icon itself is placed wrongly. |
There was a problem hiding this comment.
@mmilad75 ok, I will approve It with required follow up for the icon :)
This PR implements the Keyboard Key of the Numbered Keyboard.
fixes #16605
ios.mov
android.mov
Platforms
Steps to test