-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Ensures mini actions menu will disappear when attachment modal opens up #6083
Ensures mini actions menu will disappear when attachment modal opens up #6083
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Could you post a small video to show that it's working? Since it actually has a visual effect, but obviously it cannot be seen with a screenshot. |
@pecanoro done |
@sidferreira Could you upload videos for all platforms mentioned on the template? Your videos should reflect:
|
@parasharrajat Updated |
Thanks for uploading those. It would help us review this PR faster. I will test this later. |
@parasharrajat @mallenexpensify Did the test, but not sure if recorded them (recording was made after the tests/pr) |
Thanks, I will test it today in a few hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Tested on the web. Works well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All yours @pecanoro
@sidferreira, Great job getting your first Expensify/App pull request over the finish line! 🎉 I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:
So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Oops, I didn't notice that this PR didn't have signed commits. I am not sure what should we do now? |
@parasharrajat oh interesting, I didn't notice either, in our other repos, we block people from being able to merge PRs with unsigned commits, we should definitely enable that here too. |
Yeah, I raised this in the Slack. Let's put this suggestion there. |
@parasharrajat should I sign this too? |
I think this is not possible now. It is merged so skip this unless someone raises a flag. |
Accessibility issues found in this PR:
|
FYI we're going to revert this, I will make the same change myself so that it is signed, and then we're going to merge that PR. No action required on your parts @parasharrajat @sidferreira. Thanks for bringing it up and we'll keep a closer eye on this moving forward until we make it a requirement for the repo 😄 |
Details
The
={false}
value of the prop was forcing the mini action menu to keep visible even when it lost the focus.Fixed Issues
#5972
Tests
When tapping to open the attachment, should NOT show the mini actions menu after closing the attachments modal
QA Steps
Tested On
Recording