button styling fixed #3193

parasharrajat · 2021-05-27T21:28:53Z

Details

Fixed Issues

Tests | QA Steps

1.Open E.cash on Android and IOS.
2. Check all the above listed Pages.
3. Fixed Button button(if any) should always float over the keyboard.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

button.mp4

Android

output.mp4

parasharrajat · 2021-06-02T03:07:28Z

Requesting review @shawnborton

Dal-Papa

Apart from my comment about i18n, this looks ok to me but I'd love @marcaaron's opinion on it.

Dal-Papa · 2021-06-02T20:22:21Z

src/pages/iou/IOUCurrencySelection.js

+                                    style={[styles.textInput]}
+                                    value={this.state.searchValue}
+                                    onChangeText={this.changeSearchValue}
+                                    placeholder="Search"


Shouldn't this get translated too ?

Yeah, it should be. But It does not come under the scope of this PR. Anyways happy to do this change. Let me know, if I should do this change as well.

Could you please add it ? That way we don't forget about it.

Pushing a commit right away.

@Dal-Papa Code pushed. Thanks.

shawnborton · 2021-06-03T00:27:58Z

This looks good to me! I think we should standardize on the button styles for disabled buttons though. My thinking is that a disabled button should be 50% transparent, giving us something like this:

Thoughts?

parasharrajat · 2021-06-03T00:34:58Z

I am not sure about it. It totally depends on the Branding. I think we should raise this point on the Slack. @shawnborton

shawnborton · 2021-06-03T16:48:49Z

Well that is indeed the branding that we have designed, it just hasn't been implemented correctly. But I understand that this isn't technically part of the work we asked you to do, so we can open up a separate issue to address the button inconsistencies.

michaelhaxhiu · 2021-06-03T19:30:04Z

@shawnborton feel free to drop a comment in #expensify-open-source by saying something like ISSUE: standardize button styles and we can run it through the standard process to get it assigned!

…-button

parasharrajat · 2021-06-04T03:06:02Z

Updated. Marc is on leave till Monday. Maybe we can have it reviewed by someone else. Thanks.

Dal-Papa · 2021-06-04T18:29:44Z

@roryabraham mind giving this a quick look to be safe please ? 🙏

Dal-Papa

LGTM

parasharrajat · 2021-06-05T00:31:27Z

@roryabraham Are we good here? Thanks.

Jag96

Left a few notes but overall looks good

src/components/FixedFooter.js

src/libs/KeyboardAvoidingView/index.js

parasharrajat · 2021-06-05T01:23:34Z

@Jag96 Done.

roryabraham · 2021-06-07T16:35:24Z

I think we should standardize on the button styles for disabled buttons though. My thinking is that a disabled button should be 50% transparent, giving us something like this

Agree, but in addition we should standardize on a cursor: not-allowed style when hovering over any disabled button:

roryabraham

Overall this PR leaves me wondering if we can make some reusable components to make some of this stuff easier in the future, something like this (in psuedocode):

const ViewWithFixedButtonFooter = props => (
    <>
        <ScrollView style={[styles.flex1, styles.w100]}>
            {this.props.children}
        </ScrollView>
        {this.props.shouldShowButton && (
            <FixedFooter>
                <Button
                    success={this.props.buttonSuccess}
                    onPress={this.props.onButtonPressed}
                    style={styles.w100}
                    text={this.props.buttonText}
                />
            </FixedFooter>
        )}
    </>
)

const PageWithFixedButtonFooter = props => (
    <ScreenWrapper>
        {({didScreenTransitionEnd}) => (
            <>
                <FullScreenLoadingIndicator visible={!didScreenTransitionEnd} />
                {didScreenTransitionEnd && (
                    <KeyboardAvoidingView>
                        {this.props.shouldShowHeader && (
                            <HeaderWithCloseButton />
                        )}
                        <ViewWithFixedButtonFooter>
                            {this.props.children}
                        </ViewWithFixedButtonFooter />
                    </KeyboardAvoidingView>
                )}
            </>
        )}
    </ScreenWrapper>
)

roryabraham · 2021-06-07T16:41:11Z

src/pages/iou/IOUModal.js

-                                            selectedAmount={this.state.amount}
-                                            navigation={this.props.navigation}
-                                        />
+                                    <IOUAmountPage


All this indentation was correct, can we reset this back the way it was?

parasharrajat · 2021-06-07T18:51:19Z

Yeah. It does look that a component can be extracted but I don't think it would be that much value as it seems. Let me have a close look at it to see if we have a pattern here.

michaelhaxhiu · 2021-06-11T16:52:58Z

👋 heyo - is this on track to merge today?

parasharrajat · 2021-06-11T16:56:09Z

I am looking into extracting the code into common components but I feel like it requires some amount of time to carefully replace all the components. I can put a follow-up PR and I think we should merge this as it is working as it should be. But it's your call.

michaelhaxhiu · 2021-06-11T16:59:11Z

@parasharrajat thanks for the context. Let's have @roryabraham weigh in if he didn't already before making a decision.

But my 2 cents is we could decide this based on scope. If the addition you are describing is going to inflate scope significantly, my vote is to open a new PR. But if it's a part of the current scope then I agree with holding and keeping it contained in 1 PR for simplicity.

roryabraham · 2021-06-11T22:36:44Z

Sorry for the delay here @parasharrajat, if you resolve the merge conflicts, retest, and it's all still working then I'd feel okay approving/merging.

In my experience, all this stuff with KeyboardAvoidingView, ScrollView, and now FixedFooter is always pretty challenging to get right on all different devices. It seems like you've done a good job getting everything working (which is what we hired you for 😁 ), so we can merge this! But it would be more valuable if we could extract some reusable components from this to make the pattern easier to repeat and get right in the future. Just :my_two_cents:

parasharrajat · 2021-06-11T22:50:19Z

@roryabraham I agree with the point to extract common components but currently, I feel a little overwhelmed. I would definitely look into abstracting them. I have updated the PR and let me know what would be the best step ahead.

roryabraham

No worries @parasharrajat! I'm sure you've got higher-priority stuff that you're assigned, and this is working. Let's just move on and work on new features and bug fixes. Maybe we can circle back on this if a similar issue arises in the future 😄

OSBotify · 2021-06-11T23:22:41Z

✋ 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 · 2021-06-15T17:14:40Z

🚀 Deployed to staging in version: 1.0.68-5🚀

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

OSBotify · 2021-06-25T18:32:54Z

🚀 Deployed to production in version: 1.0.73-3🚀

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

button styling fixed

58901ec

parasharrajat requested a review from a team as a code owner May 27, 2021 21:28

MelvinBot requested review from Dal-Papa and removed request for a team May 27, 2021 21:29

parasharrajat changed the title ~~button styling fixed~~ [WIP] button styling fixed May 27, 2021

parasharrajat marked this pull request as draft May 27, 2021 21:31

parasharrajat added 3 commits May 28, 2021 04:05

fixed styling for many pages

291cce0

Pull latest changes

b767480

fix: overlapping keyboard spacer on group page

a8927aa

parasharrajat marked this pull request as ready for review June 2, 2021 03:05

parasharrajat changed the title ~~[WIP] button styling fixed~~ button styling fixed Jun 2, 2021

Dal-Papa reviewed Jun 2, 2021

View reviewed changes

Dal-Papa requested a review from marcaaron June 2, 2021 20:29

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

1f1da23

…-button

Dal-Papa requested review from roryabraham and removed request for marcaaron June 4, 2021 18:29

localized text

cf63e81

Dal-Papa previously approved these changes Jun 4, 2021

View reviewed changes

Jag96 reviewed Jun 5, 2021

View reviewed changes

src/components/FixedFooter.js Outdated Show resolved Hide resolved

src/libs/KeyboardAvoidingView/index.js Show resolved Hide resolved

tweaks

778f974

parasharrajat dismissed Dal-Papa’s stale review via 778f974 June 5, 2021 01:22

roryabraham requested changes Jun 7, 2021

View reviewed changes

parasharrajat added 2 commits June 8, 2021 07:35

fix indentation

ad3fcd0

pull latest changes

1f13dd8

Pulled latest changes

d0c316f

roryabraham assigned parasharrajat Jun 11, 2021

roryabraham approved these changes Jun 11, 2021

View reviewed changes

roryabraham merged commit c5e6da2 into Expensify:main Jun 11, 2021

OSBotify mentioned this pull request Jun 15, 2021

Deploy Checklist: Expensify.cash 2021-06-15 #3593

Closed

parasharrajat mentioned this pull request Jun 15, 2021

IOU - Request Money - Unable to write in field What's this for? #3598

Closed

NikkiWines mentioned this pull request Jun 16, 2021

mWeb - Safari- Page gets distorted tapping add phone number in profile #3604

Closed

parasharrajat mentioned this pull request Jun 21, 2021

IOU - mWeb - Page looks broken after click on Comment section #3354

Closed

roryabraham mentioned this pull request Jul 12, 2021

Give disabled buttons the disabled cursor style #3992

Merged

5 tasks

parasharrajat deleted the fixed-button branch November 4, 2022 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

button styling fixed #3193

button styling fixed #3193

parasharrajat commented May 27, 2021 •

edited

Loading

parasharrajat commented Jun 2, 2021

Dal-Papa left a comment

Dal-Papa Jun 2, 2021

parasharrajat Jun 2, 2021

Dal-Papa Jun 4, 2021

parasharrajat Jun 4, 2021

parasharrajat Jun 4, 2021

shawnborton commented Jun 3, 2021

parasharrajat commented Jun 3, 2021

shawnborton commented Jun 3, 2021

michaelhaxhiu commented Jun 3, 2021

parasharrajat commented Jun 4, 2021

Dal-Papa commented Jun 4, 2021

Dal-Papa left a comment

parasharrajat commented Jun 5, 2021

Jag96 left a comment

parasharrajat commented Jun 5, 2021

roryabraham commented Jun 7, 2021

roryabraham left a comment •

edited

Loading

roryabraham Jun 7, 2021

parasharrajat commented Jun 7, 2021

michaelhaxhiu commented Jun 11, 2021

parasharrajat commented Jun 11, 2021

michaelhaxhiu commented Jun 11, 2021

roryabraham commented Jun 11, 2021

parasharrajat commented Jun 11, 2021

roryabraham left a comment

OSBotify commented Jun 11, 2021

OSBotify commented Jun 15, 2021

OSBotify commented Jun 25, 2021

button styling fixed #3193

button styling fixed #3193

Conversation

parasharrajat commented May 27, 2021 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

parasharrajat commented Jun 2, 2021

Dal-Papa left a comment

Choose a reason for hiding this comment

Dal-Papa Jun 2, 2021

Choose a reason for hiding this comment

parasharrajat Jun 2, 2021

Choose a reason for hiding this comment

Dal-Papa Jun 4, 2021

Choose a reason for hiding this comment

parasharrajat Jun 4, 2021

Choose a reason for hiding this comment

parasharrajat Jun 4, 2021

Choose a reason for hiding this comment

shawnborton commented Jun 3, 2021

parasharrajat commented Jun 3, 2021

shawnborton commented Jun 3, 2021

michaelhaxhiu commented Jun 3, 2021

parasharrajat commented Jun 4, 2021

Dal-Papa commented Jun 4, 2021

Dal-Papa left a comment

Choose a reason for hiding this comment

parasharrajat commented Jun 5, 2021

Jag96 left a comment

Choose a reason for hiding this comment

parasharrajat commented Jun 5, 2021

roryabraham commented Jun 7, 2021

roryabraham left a comment • edited Loading

Choose a reason for hiding this comment

roryabraham Jun 7, 2021

Choose a reason for hiding this comment

parasharrajat commented Jun 7, 2021

michaelhaxhiu commented Jun 11, 2021

parasharrajat commented Jun 11, 2021

michaelhaxhiu commented Jun 11, 2021

roryabraham commented Jun 11, 2021

parasharrajat commented Jun 11, 2021

roryabraham left a comment

Choose a reason for hiding this comment

OSBotify commented Jun 11, 2021

OSBotify commented Jun 15, 2021

OSBotify commented Jun 25, 2021

parasharrajat commented May 27, 2021 •

edited

Loading

roryabraham left a comment •

edited

Loading