Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use PopoverMenu #7677

Merged
merged 6 commits into from
Mar 2, 2022
Merged

use PopoverMenu #7677

merged 6 commits into from
Mar 2, 2022

Conversation

thesahindia
Copy link
Member

@thesahindia thesahindia commented Feb 10, 2022

Details

Made payment methods container consistent

Fixed Issues

$ #7546

Tests

  • Verify that no errors appear in the JS console

QA Steps

  1. Navigate to Payments page
  2. Click add payment method
  3. Verify if the container has vertical padding
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot 2022-02-09 at 8 52 07 PM

Screen.Recording.2022-02-25.at.4.26.07.AM.mov

Mobile Web

Screenshot 2022-02-09 at 9 36 10 PM

Desktop

Can't test as there is some problem on desktop app

Screenshot 2022-02-25 at 4 39 27 AM

iOS

Screenshot 2022-02-09 at 8 49 23 PM

Android

Screenshot 2022-02-09 at 9 38 52 PM

@thesahindia thesahindia requested a review from a team as a code owner February 10, 2022 17:55
@MelvinBot MelvinBot requested review from Julesssss and rushatgabhane and removed request for a team February 10, 2022 17:55
@rushatgabhane
Copy link
Member

@thesahindia we're also adding popovermenu to storybook yeah?

@thesahindia
Copy link
Member Author

@rushatgabhane, I tried it and I am getting this error.
Screenshot 2022-02-12 at 2 59 19 AM

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 14, 2022

I am getting this error.

@thesahindia can you elaborate on what led you to this error? Steps would be appreciated, thanks!

@thesahindia
Copy link
Member Author

@rushatgabhane, I made PopoverMenu.stories.js and ran npm run storybook and got this error when tried to see the preview on localhost. The PopoverMenu uses the baseModal which uses SafeAreaInsetsContext and I think it is causing the error.

<SafeAreaInsetsContext.Consumer>
{(insets) => {
const {
paddingTop: safeAreaPaddingTop,
paddingBottom: safeAreaPaddingBottom,
paddingLeft: safeAreaPaddingLeft,
paddingRight: safeAreaPaddingRight,
} = StyleUtils.getSafeAreaPadding(insets);

@rushatgabhane
Copy link
Member

I made PopoverMenu.stories.js

@thesahindia sorry for the delay, is it possible for you to send the code for this? Thanks

@thesahindia
Copy link
Member Author

@rushatgabhane yeah sure.

import React from 'react';
import PopoverMenu from '../components/PopoverMenu';
import * as Expensicons from '../components/Icon/Expensicons';

/**
 * We use the Component Story Format for writing stories. Follow the docs here:
 *
 * https://storybook.js.org/docs/react/writing-stories/introduction#component-story-format
 */
const story = {
    title: 'Components/PopoverMenu',
    component: PopoverMenu,
};

// eslint-disable-next-line react/jsx-props-no-spreading
const Template = args => <PopoverMenu {...args} />;

// Arguments can be passed to the component by binding
// See: https://storybook.js.org/docs/react/writing-stories/introduction#using-args
const Default = Template.bind({});
Default.args = {
    isVisible: true,
    menuItems: [
        {
            text: 'Bank account',
            icon: Expensicons.Bank,
        },
        {
            text: 'Debit card',
            icon: Expensicons.CreditCard,
        },
        {
            text: 'PayPal.me',
            icon: Expensicons.PayPal,
        },
    ],
};

export default story;
export {
    Default,
};

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 16, 2022

@thesahindia i think you need to pass anchor position

// Anchor position is optional only because it is not relevant on mobile

@thesahindia
Copy link
Member Author

@rushatgabhane, I tried passing anchor position but it didn't work.

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 22, 2022

Oh man, I can't get it to work either or find the root cause. I tried using ButtonWithMenu too, but I get the same error.

@thesahindia could you please ask about this on slack? Thanks!

@rushatgabhane
Copy link
Member

@thesahindia looks like we have a solution. Let's get it done :)

@thesahindia
Copy link
Member Author

thesahindia commented Feb 24, 2022

@rushatgabhane PR is almost ready for review, just need to fix some lint errors, will do it soon. Done.

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Julesssss LGTM! 🎉️

@Julesssss
Copy link
Contributor

Julesssss commented Mar 2, 2022

I noticed this issue during testing, but it's unrelated as it already exists on prod.

Screenshot 2022-03-02 at 11 34 41

@thesahindia
Copy link
Member Author

thesahindia commented Mar 2, 2022

I noticed this issue during testing, but it's unrelated as it already exists on prod.

Hmm strange I had reported this issue on slack but I found that it won't need a separate issue as the current PR was going to solve it.
Edit:- I checked it again and it's working fine after the changes of current PR

@Julesssss
Copy link
Contributor

Oh, let me try again. Maybe the mobile build didn't install

@Julesssss
Copy link
Contributor

@thesahindia yep, you're correct. Not sure what happened there 😕

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Julesssss Julesssss merged commit ea702fa into Expensify:main Mar 2, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2022

✋ 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
Copy link
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.1.42-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants