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

Revert "Commenting out the confimation modal to test something in staging." #8892

Merged
merged 1 commit into from
May 6, 2022

Conversation

sketchydroide
Copy link
Contributor

Reverts #8882

It did not fix #8791 reverting

@sketchydroide sketchydroide self-assigned this May 6, 2022
@sketchydroide sketchydroide requested a review from a team as a code owner May 6, 2022 11:40
@melvin-bot melvin-bot bot requested review from MariaHCD and removed request for a team May 6, 2022 11:40
Copy link
Contributor

@MariaHCD MariaHCD left a comment

Choose a reason for hiding this comment

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

LGTM! Two questions:

  1. What were the results of the test?
  2. Does this PR need to be CP'd as well?

@sketchydroide sketchydroide added Internal Requires API changes or must be handled by Expensify staff InternalQA This pull request required internal QA labels May 6, 2022
@sketchydroide
Copy link
Contributor Author

sketchydroide commented May 6, 2022

The result of the test was that the issue was still ocurring. And the changes seems to have no effect on the issue. This was an exploratory PR due to the ficulties of testing this on dev.
It will need to CP, but I'll do it as I'm the deployer of the week

(edit) forgo that I could add the CP label

@github-actions
Copy link
Contributor

github-actions bot commented May 6, 2022

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@MariaHCD MariaHCD merged commit a29cdc2 into main May 6, 2022
@MariaHCD MariaHCD deleted the revert-8882-afonseca_remove_confimation_workspace branch May 6, 2022 12:53
@melvin-bot
Copy link

melvin-bot bot commented May 6, 2022

Triggered auto assignment to @joelbettner (InternalQA), see https://stackoverflow.com/c/expensify/questions/5042 for more details.

@melvin-bot
Copy link

melvin-bot bot commented May 6, 2022

@MariaHCD looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@MariaHCD
Copy link
Contributor

MariaHCD commented May 6, 2022

Merged since this was a straight revert.

@MariaHCD MariaHCD removed the Emergency label May 6, 2022
OSBotify pushed a commit that referenced this pull request May 6, 2022
…onfimation_workspace

Revert "Commenting out the confimation modal to test something in staging."

(cherry picked from commit a29cdc2)
OSBotify added a commit that referenced this pull request May 6, 2022
@OSBotify
Copy link
Contributor

OSBotify commented May 6, 2022

🚀 Cherry-picked to staging by @MariaHCD in version: 1.1.57-7 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[$4000] mWeb/safari - Menu - "Open app banner" at the top causes spacing issues at the bottom of the browser
4 participants