Rename TextInputFocusable to `Composer` ⌨️ & add Stories For TextInputs and fix bugs #7984

parasharrajat · 2022-03-02T17:54:46Z

Details

Fix bugs with TextInput.
Add Stories for Composer & TextInput variants.
Rename TextInputFocusable to Composer to better reflect its purpose.

Fixed Issues

$ #6584

Tests

Tested the message sending functionality.
Tested EmojiPicker.
Add Stories and test that they are working fine without errors.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Open the app on test the following steps on each platform.
Check that you are able to send messages as before on all platforms. You are able to use Markdown as before to compose the message.

Open Stories on Staging Web app via https://staging.new.expensify.com/docs/index.html.
Check if it has the following stories (title can slightly differ from below mentioned items):

 -  TextInput/AutoGrow,
    TextInput/PrefixedInput,
    TextInput/ForceActiveLabel,
    TextInput/MaxLengthInput,
    TextInput/HintAndErrorInput,
    Composer/DefaultInput

Test these stories. They should work as titled.

Go to the request Money Page and try to add the amount. It should work as before. Test it on all platforms.
Go to Profile Page, Try to change Profile settings and details. It should work as before.

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat · 2022-03-05T16:31:12Z

Found one issue. MaxLength Counter is not incrementing automatically when a user types the value in the Story. The reason is that when we don't bound the value prop to state on the parent component, TextInput never renders on the new value as it does not use state. We have to move the value property to state so that increments properly.

But this issue is not linked with this PR and thus I request another milestone on the current contract to fix this.

rushatgabhane · 2022-03-07T14:09:36Z

@parasharrajat this is ready for review yeah? let's remove the hold then

parasharrajat · 2022-03-07T14:29:56Z

I will update the screenshots and then remove the hold.

parasharrajat · 2022-03-07T19:40:48Z

Leaving a note so that I can fix those into the same milestone #7984 (comment)
Another issue: When the label is forced to be active. Placeholder text is not visible until you focus the input.

parasharrajat · 2022-03-07T21:50:58Z

@rushatgabhane Do you want me to fix above-mentioned issues in the current PR or create a new PR?

rushatgabhane · 2022-03-07T22:12:34Z

@parasharrajat let's create a new PR, it'll be easier to read if someone comes back to it later.

parasharrajat · 2022-03-07T22:13:30Z

Then it's good on hold. This is supposed to be final PR.

parasharrajat · 2022-03-07T22:15:32Z

Issue: Autogrow does not work for unbound inputs.

parasharrajat · 2022-03-07T22:20:47Z

I think it makes sense to keep them here as they won't make sense without stories.

rushatgabhane · 2022-03-07T23:05:49Z

Cool, I'll review this tomorrow

rushatgabhane

Looks good
@parasharrajat idk how to comment on files that aren't modified

But just rename these to composer, thanks!

parasharrajat · 2022-03-09T15:49:28Z

Yeah. Missed that.

…nput-final

rushatgabhane

@parasharrajat compose box disappears when editing comment.
This isn't present on main

Screen.Recording.2022-03-09.at.7.09.44.PM.mov

parasharrajat · 2022-03-09T16:42:19Z

This is on main. And I don't see how this is related to changes on this PR.

rushatgabhane · 2022-03-09T17:08:09Z

You're right. Sorry, I was mistaken

rushatgabhane

@NikkiWines LGTM! 🎉

NikkiWines

Couple small comments. I think since this is changing so much your test and QA steps need to be more robust @parasharrajat

src/styles/addOutlineWidth/index.js

src/components/TextInput/baseTextInputPropTypes.js

src/stories/Composer.stories.js

src/components/TextInput/BaseTextInput.js

NikkiWines · 2022-03-09T23:01:31Z

src/stories/TextInput.stories.js

+            // eslint-disable-next-line react/jsx-props-no-spreading
+            {...args}
+            onChangeText={(value) => {
+                if (value && value.toLowerCase() === 'damn!') {


Why damn!? 😄

Just an example. 🤣 It fits into Damn! there is an error. hint messages.

😅 I think it's probably better to use something like Oops! Looks like there's an error.

src/components/TextInput/BaseTextInput.js

parasharrajat · 2022-03-11T08:06:10Z

Updated.

NikkiWines

Sorry noticed while running through the test steps that we're getting fairly inconsistent in TextInput.stories.js - let's try to have standard conventions there.

Also, I would update the QA steps to identify how to get to Stories

NikkiWines · 2022-03-11T19:16:45Z

src/stories/TextInput.stories.js

+const AutoGrow = Template.bind({});
+AutoGrow.storyName = 'Autogrow input';
+AutoGrow.args = {
+    label: 'Autogrow input',


Let's standardize the case-convention for the labels: AutoGrow Input since the other labels seem to capitalize everything.

It seems older ones have first as only capital letter so I am going with that following the page titles in the app.

It looks like there are still two main conventions for labels:

Input with xyz - e.g. Input with default value

xyz input - e.g. Prefixed input

I'm fine with either options but i do think we should pick one

Updated most of them. Left few whose name makes more sense. But I would say it does not matter much. The main perspective should be that they should clearly show the proper example of that component/varient

src/stories/TextInput.stories.js

NikkiWines · 2022-03-11T19:20:24Z

src/stories/TextInput.stories.js

+        />
+    );
+};
+HintErrorInput.storyName = 'Input with hint & error';


HintAndError Input?

parasharrajat · 2022-03-11T20:47:10Z

Updated. I don't remember the exact URL to open the stories on staging. Could you please let me know so that I can update that in QA steps?

NikkiWines

Couple of other comments, but since you've updated the ones you added I'm happy to make another PR to handle standardizing some of the older inputs.

I'll approve but will re-review if you want to implement the suggested changes

src/stories/TextInput.stories.js

NikkiWines · 2022-03-11T22:20:42Z

src/stories/TextInput.stories.js

+const AutoGrow = Template.bind({});
+AutoGrow.storyName = 'Autogrow input';
+AutoGrow.args = {
+    label: 'Autogrow input',


It looks like there are still two main conventions for labels:

Input with xyz - e.g. Input with default value

xyz input - e.g. Prefixed input

I'm fine with either options but i do think we should pick one

NikkiWines · 2022-03-11T22:27:00Z

Updated. I don't remember the exact URL to open the stories on staging. Could you please let me know so that I can update that in QA steps?

yeah! the URL is https://staging.new.expensify.com/docs/index.html

NikkiWines

👍 Thanks for updating those

NikkiWines · 2022-03-11T23:18:47Z

Gonna merge as @rushatgabhane already approved earlier

OSBotify · 2022-03-11T23:27:25Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-03-15T21:36:45Z

🚀 Deployed to staging by @NikkiWines in version: 1.1.43-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-03-17T17:56:42Z

🚀 Deployed to production by @roryabraham in version: 1.1.43-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mananjadhav · 2023-04-27T09:47:13Z

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

parasharrajat requested a review from a team as a code owner March 2, 2022 17:54

parasharrajat changed the title ~~Rename TextInput Focusable to Composer ⌨️ & add Stories~~ [HOLD] Rename TextInput Focusable to Composer ⌨️ & add Stories Mar 2, 2022

MelvinBot requested review from NikkiWines and rushatgabhane and removed request for a team March 2, 2022 17:55

parasharrajat added 4 commits March 5, 2022 06:28

Rename TextInputFocusable to Composer

5aaced4

Add TextInput Stories

cd2f82c

fix param type for story

b8be9ce

Story for Composer

3efd5e2

parasharrajat force-pushed the textinput-final branch from a4e7316 to 3efd5e2 Compare March 5, 2022 00:58

parasharrajat added 6 commits March 8, 2022 03:51

Fix: autogrow Story

f9ad098

Move value to state to trigger updates on value change

4955180

Fix: AutoGrow Input styling

6c1c388

Visually Hide internal Text

a7407a6

Add comments

ab066cb

Show the placeholder when forceActiveLabel is true

564b55c

parasharrajat changed the title ~~[HOLD] Rename TextInput Focusable to Composer ⌨️ & add Stories~~ Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs Mar 7, 2022

rushatgabhane reviewed Mar 9, 2022

View reviewed changes

Merge branch 'main' of github.com:Expensify/Expensify.cash into texti…

301e605

…nput-final

rushatgabhane suggested changes Mar 9, 2022

View reviewed changes

rushatgabhane previously approved these changes Mar 9, 2022

View reviewed changes

NikkiWines requested changes Mar 9, 2022

View reviewed changes

Clean up and refactor

ec717fe

parasharrajat dismissed rushatgabhane’s stale review via ec717fe March 10, 2022 11:06

Change error message

6c2f5c7

NikkiWines requested changes Mar 11, 2022

View reviewed changes

parasharrajat added 2 commits March 12, 2022 02:14

clean up Stories

8a7d658

fix labels

b066c8d

NikkiWines previously approved these changes Mar 11, 2022

View reviewed changes

Update stories

ee1ecba

parasharrajat dismissed NikkiWines’s stale review via ee1ecba March 11, 2022 22:46

NikkiWines approved these changes Mar 11, 2022

View reviewed changes

NikkiWines merged commit a613b2b into Expensify:main Mar 11, 2022

OSBotify mentioned this pull request Mar 15, 2022

Deploy Checklist: New Expensify 2022-03-15 #8170

Closed

52 tasks

kbecciv mentioned this pull request Mar 16, 2022

Web - Endless spinner is displayed when try to open Stories #8179

Closed

botify mentioned this pull request Mar 17, 2022

[HOLD for payment 2022-04-01] Simplify/consolidate our various Text Input components #6584

Closed

SumitDiyora mentioned this pull request Apr 9, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

alexxxwork mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [HOLD for payment 2023-04-20] [$1000] Text strings must be rendered within a <Text> component #16683

Closed

parasharrajat deleted the textinput-final branch November 20, 2023 13:08

Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984

Rename TextInputFocusable to Composer ⌨️ & add Stories For TextInputs and fix bugs #7984

Conversation

parasharrajat commented Mar 2, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat commented Mar 5, 2022

rushatgabhane commented Mar 7, 2022

parasharrajat commented Mar 7, 2022

parasharrajat commented Mar 7, 2022

parasharrajat commented Mar 7, 2022

rushatgabhane commented Mar 7, 2022 • edited Loading

parasharrajat commented Mar 7, 2022

parasharrajat commented Mar 7, 2022

parasharrajat commented Mar 7, 2022

rushatgabhane commented Mar 7, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Mar 9, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Mar 9, 2022

rushatgabhane commented Mar 9, 2022

rushatgabhane left a comment

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Mar 11, 2022

NikkiWines 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

parasharrajat commented Mar 11, 2022 • edited Loading

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikkiWines commented Mar 11, 2022

NikkiWines left a comment

Choose a reason for hiding this comment

NikkiWines commented Mar 11, 2022

OSBotify commented Mar 11, 2022

OSBotify commented Mar 15, 2022

OSBotify commented Mar 17, 2022

mananjadhav commented Apr 27, 2023

Rename TextInputFocusable to `Composer` ⌨️ & add Stories For TextInputs and fix bugs #7984

Rename TextInputFocusable to `Composer` ⌨️ & add Stories For TextInputs and fix bugs #7984

parasharrajat commented Mar 2, 2022 •

edited

Loading

rushatgabhane commented Mar 7, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane left a comment •

edited

Loading

NikkiWines Mar 11, 2022 •

edited

Loading

parasharrajat commented Mar 11, 2022 •

edited

Loading