Create popup component, show it when profile changes is saved #2944
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
src/libs/PopUpNotification.js
Outdated
| ...styles.p5, | ||
| }, | ||
| bodyText: { | ||
| fontSize: variables.fontSizeNormal, |
There was a problem hiding this comment.
Just noticing this now, but I think you also need to pull in the correct fontFamily as well. Should be fontFamily: fontFamily.GTA,
src/styles/colors.js
Outdated
| @@ -10,5 +10,6 @@ export default { | |||
| green: '#03d47c', | |||
| greenHover: '#03c775', | |||
| red: '#fc3826', | |||
| yellow: '#FED607', | |||
There was a problem hiding this comment.
Minor comment but can we use all lowercase here for consistency? #fed607
|
Awesome, thanks for all of the changes. Could we also see the growl notification when there is at least two lines of text? |
With default (19px) line height With 24px line heigth Which one would you like me to commit? |
|
A line height of |
|
Can we rename |
…styles to styles.js
Sure |
|
@kakajann It looks like you've got a merge conflict on this pull request |
|
Also, since this is your first time contributing to Expensify.cash (and we're thrilled to have you, by the way 😁), please be sure to read over the Contributor License Agreement and digitally sign it when you get a chance. Thanks! |
Co-authored-by: Alexander Mechler <alex@mechler.dev>
|
I don't want to leave a full review here, but noticed we are not capitalizing types for JSDoc Style guide here if anyone needs them |
|
|
||
| const propTypes = { | ||
| children: PropTypes.node.isRequired, | ||
| translateY: PropTypes.instanceOf(Animated.Value).isRequired, |
There was a problem hiding this comment.
These also need comments.
|
|
||
| const propTypes = { | ||
| children: PropTypes.node.isRequired, | ||
| translateY: PropTypes.instanceOf(Animated.Value).isRequired, |
There was a problem hiding this comment.
also could probably be generalized since they are the same in both files.
There was a problem hiding this comment.
Sure, you are right, I just generalized them.
…ify.cash into create-popup-component
src/libs/GrowlNotification/GrowlNotificationContainer/GrowlNotificationContainerPropTypes.js
Outdated
Show resolved
Hide resolved
marcaaron
changed the title
|
@roryabraham @alex-mechler put a hold on this since @thienlnam brought to my attention that the author has not been hired. |
There was a problem hiding this comment.
Hey @kakajann,
While this is great work, you're not currently hired for the Upwork job and have skipped a couple of important steps in the process.
Under Propose a solution for the job, a couple of the steps are to
- Pause at this step until Expensify provides feedback on your proposal (do not begin coding or creating a pull request yet).
- If your solution proposal is accepted, Expensify will hire you on Upwork and assign the GitHub issue to you.
Your solution proposal was not accepted and you have not been hired on upwork. In this case, there aren't any other people working on this issue so we'll let you have this but please follow the steps in the future and reread the contributing guidelines.
thienlnam
changed the title
|
Merging since it looks like Marcs latest comment was resolved and both Rory and I have approved |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@kakajann Congrats on getting your first Expensify.cash pull request merged! Great job 🎉 |
Thank you, it was a great experience for me to work with you. |
Details
Notification pops up when a user make a change in their profile and saves it.
Changes
PopUpNotificationcomponent. It has 3 types (success, error, warning) and swipe or click to dismiss feature.profilePage.popupMessageOnSavetoen.js/assets/images/exclamation.svgFixed Issues
Fixed: #2812
Tests
Tested in all 5 platforms. I will attach videos below.
Tested On
Screenshots
Web
Web.mov
Mobile Web
web.mobile.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov