feat: add title input component to quo2 #15133
Conversation
Jenkins BuildsClick to see older builds (33)
|
| max-length 24 | ||
| on-change-text (fn [v] | ||
| (reset! value v) | ||
| (reagent/flush) |
There was a problem hiding this comment.
without reagent/flush the text flickers on input..
researched this here:
reagent-project/reagent#485 (comment)
Should we take a different approach for input components, I see quo text inputs are quite complicated although perhaps this is necessary for a smooth input effect.
In any case we should probably have the base functionality available for all input components and then we can stylise the ui as needed for the various components.
Would appreciate some input (🤓) on this
There was a problem hiding this comment.
:default-value should be used, we shouldn't pass any value to the input
There was a problem hiding this comment.
how do you mean? surely the on change has to pass the updated value to the reset function ?
There was a problem hiding this comment.
https://gist.github.com/ilmotta/9753c003359bda9be2afd85d4e4a78d9
@J-Son89, see the gist I created now. I tested now and it works correctly. It follows some of the suggestions by @flexsurfer
| (ns quo2.components.inputs.title-input.style | ||
| (:require [quo2.foundations.colors :as colors])) | ||
|
|
||
|
|
| [disabled? blur?] | ||
| {:color (when disabled? (get-disabled-color blur?))}) | ||
|
|
||
|
|
| max-length 24 | ||
| on-change-text (fn [v] | ||
| (reset! value v) | ||
| (reagent/flush) |
There was a problem hiding this comment.
:default-value should be used, we shouldn't pass any value to the input
|
|
||
| (defn- pad-0 | ||
| [value] | ||
| (if (<= (count value) 1) |
There was a problem hiding this comment.
im not sure this is needed, i guess its just in design for the demo purposes, there is an example 5/24 and there is no leading zero
There was a problem hiding this comment.
I think I meant 9. it should always be two digits. e.g 05/24 or 03/09 etc
Will check again to make sure it's right 👍
There was a problem hiding this comment.
better to ask in figma, to make sure
There was a problem hiding this comment.
yes, I will double check with design team 👍
There was a problem hiding this comment.
FYI: If we want to format with leading zeros, in CLJS we should probably use format:
(goog.string/format "%02d" 9) ; => 09There was a problem hiding this comment.
@ilmotta, this caused issues with the component tests. The compiler makes goog a global object so it doesn't play so nice with the tests. There's most likely a way to set this up, and something we will definitely need to address at some point.
For now I left is as I would prefer to keep the tests on this and it's only some basic functionality but I will spend some time looking into how we can work with goog imports and component tests.
TypeError: goog.string.format is not a function
There was a problem hiding this comment.
No problem then, thanks for the info. It could be a good idea to create a quick issue if we haven't one.
There was a problem hiding this comment.
Yeah sure I can create an issue for this 👍
| :on-change-text on-change-text | ||
| :disabled? (:disabled? @state) | ||
| :blur? (:blur? @state) | ||
| :placeholder "type something here"} @value]]]]))) |
There was a problem hiding this comment.
i mean here, we shouldn't pass @value
There was a problem hiding this comment.
hmm, that seems strange to me. is there not times then when the ui and data is out of sync?
There was a problem hiding this comment.
it should be :default-value @value
There was a problem hiding this comment.
hmm, that seems strange to me. is there not times then when the ui and data is out of sync?
why strange? why would you want to set value ? it was already set when you pressed the key
There was a problem hiding this comment.
I agree with @flexsurfer
Just to make more clear the explanation:
[my-text-component
{:on-change #(reset! @value,,,) ;; <- or any other function you want
:default-value @value ;; <- this could be useful to initialize the field
;; with a specified value, but it's not required
:value @value ;; <- you set this only if you manually want to update
;; component's value. e.g. you have a "clear" button that
;; when pressed it resets the atom to "", if you don't
;; specify `:value @value`, then your change will not be
;; reflected on the input.
}]
J-Son89
requested review from
Parveshdhull,
OmarBasem,
smohamedjavid,
erikseppanen,
ibrkhalil,
ilmotta and
ulisesmac
|
Hey @J-Son89 |
I'm sorry, I thought you were implementing the "Profile Input" component instead of "Title Input". |
src/quo2/foundations/colors.cljs
Outdated
| @@ -39,6 +39,14 @@ | |||
| (alpha light-color light-opacity) | |||
| (alpha dark-color dark-opacity)))))) | |||
|
|
|||
| (defn theme-color-with-blur | |||
There was a problem hiding this comment.
I'm not sure about adding this abstraction 🤔
AFAIK, we don't dinamically change from blur colors to non-blur colors as we do with dark and light colors.
I've seen the usages in your component's styles and it looks good, but I would first put it as a helper function in your style namespace, if the patttern is repeated more often in other components, then add to a more general namespace (such as foundations/colors).
My main concern is that function could lead to the understanding that we dinamically change from blur to non-blur colors and it could easlily be rewritten from:
(defn- get-char-count-color
[blur?]
(colors/theme-color-with-blur
blur?
colors/neutral-40
(colors/alpha colors/neutral-80 0.4)
colors/neutral-50
(colors/alpha colors/white 0.4)))to:
(defn- get-char-count-color
[blur?]
(if blur?
(colors/theme-colors (colors/alpha colors/neutral-80 0.4) (colors/alpha colors/white 0.4))
(colors/theme-colors colors/neutral-40 colors/neutral-50)))Would be nice to have input from other members :)
There was a problem hiding this comment.
yep, I've just seen across multiple components where we have 4 variations of colors (light, dark, light +blur, dark + blur).
It's pretty common across the designs really, see "src/quo2/components/selectors/styles.cljs" and it could use this function there.
Re: "AFAIK, we don't dinamically change from blur colors to non-blur colors as we do with dark and light colors."
Not sure what you mean by - dynamically changing color sets?
most of the blur variations have their own colors than their respective light or dark non blur implementation.
There was a problem hiding this comment.
but yep, happy to adjust this helper and also localize it to this style file if other devs see this to be the case too 👍
There was a problem hiding this comment.
Not sure what you mean by - dynamically changing color sets?
I mean, a component using (theme-colors light-color dark-color) will return light-color or dark-color depending on user preferences (dinamically).
But we won't swap between a light-color and a blur-light-color, I mean, a component using a blur color will always use a blur color, and it'll swap between blur-light-color and blur-dark-color which is already covered by the theme-colors fn.
| [rn/view | ||
| {:style style/container} | ||
| [rn/view | ||
| [rn/text-input |
There was a problem hiding this comment.
The cursor's color is different when we change the theme and blur variants:
(blue, black and white, for example):
There was a problem hiding this comment.
I've seen some problems while testing on Android: Pixel4 API 30:
the sentence is incomplete but there is still room for it to fit.
-
It seems "00/24" is a little lower than the designs, but IDK whether this is related to the implementation or phone's font size
-
A strange "." appears in the placeholder after you write:
Screencast from 2023-02-21 14-22-57.webm
There was a problem hiding this comment.
I will take a look, think I need to re-approach that alignment. Will adjust the cursor too!
yep, my bad. I thought I had created the other issue. Created the correct issue and adjusted the link! |
| [rn/view {:flex 1}] | ||
| [preview/customizer state descriptor] | ||
| [rn/view | ||
| {:padding-vertical 60 |
There was a problem hiding this comment.
No big deal, but these props should be inside the :style map.
There was a problem hiding this comment.
yes,I will add this and separate into view/style files.
I think we should note however that none of the other preview screens have followed this best practice so far, not even new ones.
Ideally we will move preview screens to a better tool (most likely Storybook ) in which case these files will all be removed, and we can clean up the preview screens code somewhat at the same time too.
There was a problem hiding this comment.
I've attempted once to add Storybook to status-mobile, but never really got it to a decent state to share. Would love to see progress in this area too.
| :size :paragraph-2} (pad-0 (str (count value)))] | ||
| [text/text | ||
| {:style (style/char-count blur?) | ||
| :size :paragraph-2} (str "/" (pad-0 (str max-length)))]]]]))) |
There was a problem hiding this comment.
| (colors/theme-color-with-blur | ||
| blur? | ||
| colors/neutral-30 | ||
| (colors/alpha colors/neutral-80 0.2) |
There was a problem hiding this comment.
Why not colors/neutral-80-opa-20?
| :flex 1} | ||
| [rn/flat-list | ||
| {:flex 1 | ||
| :keyboardShouldPersistTaps :always |
There was a problem hiding this comment.
Should this be kebab-case?
J-Son89
force-pushed
the
jc/title-input
branch
3 times, most recently
from
b15aa82
to
58443b5
Compare
|
|
||
| (defn- pad-0 | ||
| [value] | ||
| (if (<= (count value) 1) |
There was a problem hiding this comment.
FYI: If we want to format with leading zeros, in CLJS we should probably use format:
(goog.string/format "%02d" 9) ; => 09| {:label "Cursor Color" | ||
| :key :color | ||
| :type :select | ||
| :options (map (fn [color] (let [key (get color :name)] {:key key :value key})) |
There was a problem hiding this comment.
One-liners can easily become unreadable in Lisps, because we lose track of the code (also data) structure. I don't know about you folks, but I find the code below more readable.
(map (fn [color]
(let [key (get color :name)]
{:key key :value key}))
(quo/picker-colors))
There was a problem hiding this comment.
@J-Son89 this may squeak by as a one-liner :)
(map (fn [{:keys [name]}] {:key name :value name}) (quo/picker-colors))| :customization-color (:color @state) | ||
| :blur? (:blur? @state) | ||
| :placeholder "type something here"}]] | ||
|
|
| :style (style/title-text disabled? blur?)}) | ||
| :default-value default-value | ||
| :accessibility-label :profile-title-input | ||
| :on-focus #(swap! focused? not) |
There was a problem hiding this comment.
Shouldn't :on-focus be #(reset! focused? true) and :on-blur be #(reset! focused? false) ? For example, in which situation would we want to have focused? true if the input is not focused and vice-versa?
There was a problem hiding this comment.
yeah better to be safe about this 👍
J-Son89
force-pushed
the
jc/title-input
branch
2 times, most recently
from
cd16b05
to
629b902
Compare
| @@ -116,6 +116,20 @@ deref'ed atoms. | |||
| {:background-color (colors/theme-colors colors/white colors/neutral-90)}) | |||
| ``` | |||
|
|
|||
| ### Custom Colors | |||
|
|
|||
There was a problem hiding this comment.
@Parveshdhull updated the docs for customization color :)
J-Son89
force-pushed
the
jc/title-input
branch
2 times, most recently
from
9995729
to
a191675
Compare
J-Son89
force-pushed
the
jc/title-input
branch
2 times, most recently
from
27c6259
to
79daa93
Compare
|
@ulisesmac I updated my changes with this branch so the preview screen should be correct on android now |
fixes: #15159
implemented quo2 component for profile input.
dark placeholder focused
dark placeholder not focused (blur)
dark text completely filled in