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

Make inserter to show options on long-press to add before/after #1645

Closed
wants to merge 20 commits into from
Closed

Make inserter to show options on long-press to add before/after #1645

wants to merge 20 commits into from

Conversation

ceyhun
Copy link
Contributor

@ceyhun ceyhun commented Dec 3, 2019

Fixes #1433

Gutenberg PR: WordPress/gutenberg#18791

For detailed test cases and screenshots, please check out WordPress/gutenberg#18791

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@SergioEstevao
Copy link
Contributor

SergioEstevao commented Dec 11, 2019

@ceyhun do you mind to update the branch with the latest changes from develop?

We have some updates there that sort some of the issue with the e2e tests, that I would like to see running on this PR :).

@ceyhun
Copy link
Contributor Author

ceyhun commented Dec 17, 2019

@SergioEstevao I merged develop and also rebased gutenberg PR onto develop's hash. Still all e2e tests are failing.

@mchowning
Copy link
Contributor

mchowning commented Dec 18, 2019

@SergioEstevao I merged develop and also rebased gutenberg PR onto develop's hash. Still all e2e tests are failing.

👋 @ceyhun! I know that it used to be the case that e2e tests would fail for PRs from forks (comment on PR from April with similar issue). I'm guessing that is why you updated the submodule ref to point at your gutenberg fork. I nevertheless pushed your code to branches with the same name on gutenberg-mobile and gutenberg to see if it would make any difference, but the Android device tests are still failing. 😞

@ceyhun
Copy link
Contributor Author

ceyhun commented Dec 18, 2019

Hello @mchowning 🤔 Before it was failing on both iOS and Android, last test run seems to be behaving differently now. Let me see if I reproduce this when I run locally.

@ceyhun
Copy link
Contributor Author

ceyhun commented Dec 18, 2019

@mchowning I was able to reproduce locally. So I merged develop and also changed the gutenberg ref to the most recent changes of my gutenberg PR and then I ran again locally. Some of them passed this time but there are still failing ones. Then I tried running e2e tests locally on the develop branch of gutenberg-mobile instead of my branch and it's the same result. Could develop of gutenberg-mobile be broken or is there some flakiness involved when ran locally?

Can you please try again with these changes I pushed? Maybe they fail locally but will run successfully on CI?

@mchowning
Copy link
Contributor

And now the device tests for both platforms in addition to the jest tests for ios are failing. 😱

I pushed your code to the project repo and the device tests are passing, but it's got a single failure with the jest tests on iOS. 😕 I can reproduce that failure locally if I run TEST_RN_PLATFORM=ios yarn test (have to manually set the platform to ios since it defaults to android otherwise). It seems like the device tests are likely flakiness/fork issues, so let's see if we can get all the jest tests passing on both platforms.

When I run the jest tests locally I only see the same single iOS test failure that CI is reporting, so if you're seeing more failures than that, let me know. In that case there may be some kind of setup issue we need to resolve.

@ceyhun
Copy link
Contributor Author

ceyhun commented Dec 18, 2019

Thanks for trying again @mchowning :) For some reason I don't understand yet

<>
  <Component/>
</>

when rendered to JSON returns an array on Android but an object on iOS.
So instead of using toJSON method of react-test-renderer I used the findByProps method. Now it should pass on both platforms.

@mchowning
Copy link
Contributor

I just merged develop into the branch I created with your code since we have a fix for the runtime crash you saw merged into develop now. Let's see how that build does.

@ceyhun
Copy link
Contributor Author

ceyhun commented Dec 20, 2019

Thanks again @mchowning 👍 I had a conflict in my gutenberg PR so I rebased it and also merged develop here again. Android issues seem to be fixed now for me.

@ceyhun
Copy link
Contributor Author

ceyhun commented Jan 29, 2020

Currently having issues running iOS and Android device tests on forks. Opened a new PR from gutenberg-mobile branch as a workaround until it's fixed: #1835

@ceyhun ceyhun closed this Jan 29, 2020
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.

Proposal: Long-press on inserter to show options for "add above" and "add below"
3 participants