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.

Sign up for GitHub

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

Jump to bottom

MediaUpload and MediaPlaceholder unify props #1310

Merged
merged 7 commits into from
Aug 30, 2019
Merged

MediaUpload and MediaPlaceholder unify props #1310

merged 7 commits into from
Aug 30, 2019

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Aug 22, 2019 •
edited
Loading

This PR unifies web and mobile props for MediaUpload and MediaPlaceholder components. It is the first iteration where we do not support multiple select images/videos.

Related gutenberg PR - WordPress/gutenberg#17145
See #1298 and #1299

@pinarol pinarol requested review from pinarol and koke August 22, 2019 16:16
@pinarol pinarol added the Media label Aug 22, 2019
@pinarol
Copy link
Contributor

pinarol commented Aug 22, 2019

@dratwas Could you please put the link of the associated gutenberg PR in the description please? Thanks!

@dratwas dratwas mentioned this pull request Aug 23, 2019
Merged
5 tasks
@pinarol
Copy link
Contributor

pinarol commented Aug 23, 2019

Thanks for the PR! I'll start reviewing this but, a couple of things that caught my attention:

I think the .gitmodules file needs this change in order for reviewers to see your fork's changes:

Screen Shot 2019-08-23 at 17 09 44

But this should be reverted before the merge ☝️

Secondly, you should commit & push gutenberg subrepo hash to this PR after you pushed things to the gutenberg PR.

@pinarol
Copy link
Contributor

pinarol commented Aug 23, 2019

That way reviewer can checkout the PR branch and start testing right after just running a submodule update.

@koke
Copy link
Member

koke commented Aug 23, 2019

I tried checking it out locally and it worked for me, even if it fails in CI. I think github exposes external commits that are part of a PR when fetching somehow. My guess is that it might be something that only works on newer git clients, but I don’t really know how that’s working.

@pinarol
Copy link
Contributor

pinarol commented Aug 26, 2019

To me it was confusing and limiting. First of all I got an error on checkout so it left me with some question marks about having the right things to test. Our CI has the same error: Fetched in submodule path 'gutenberg', but it did not contain 23ab43f458464675aef9fe10b13796e24bf4b9b3. Direct fetching of that commit failed. I wanted to go to the subrepo and check if I have the updated version of the branch but I couldn't checkout the remote branch because it didn't exist. After changing my subrepo config I was able to do this and realized that this PR had the old hash pointing to gutenberg repo.

I think it would contain less confusion that way and CI jobs would also succeed hopefully.

@dratwas dratwas force-pushed the callstack/media-upload-props branch from 0aa8a5a to f516611 Compare August 28, 2019 11:04
@dratwas dratwas closed this Aug 28, 2019
@dratwas dratwas force-pushed the callstack/media-upload-props branch from d35724a to fb7dc7d Compare August 28, 2019 11:18
@dratwas dratwas reopened this Aug 28, 2019
pinarol
pinarol approved these changes Aug 28, 2019
View reviewed changes
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

LGTM! Tested as described here: WordPress/gutenberg#17145 (review)

After the gutenberg PR is merged please update the gutenberg submodule ref to point to rnmobile/master and revert the .gitmodules file.

@koke
Copy link
Member

koke commented Aug 30, 2019

WordPress/gutenberg#17145 was just merged

@dratwas
Copy link
Contributor Author

dratwas commented Aug 30, 2019

After the gutenberg PR is merged please update the gutenberg submodule ref to point to rnmobile/master and revert the .gitmodules file.

Done :)

@pinarol
Copy link
Contributor

pinarol commented Aug 30, 2019

● Gutenberg Editor paste tests › copies styled text from one paragraph block and pastes in another

@etoledom is this the failing UI test you were referring to ? if so I'll merge this one.

@pinarol
Copy link
Contributor

pinarol commented Aug 30, 2019

@dratwas Could you be able to update gutenberg subrepo again to point to the latest rnmobile/master ? We just merged and WordPress/gutenberg#17268 it is related with this work

@etoledom
Copy link
Contributor

@etoledom is this the failing UI test you were referring to ? if so I'll merge this one.

Yes, it looks like it. The best way to be sure is to run them locally though.

@pinarol
Copy link
Contributor

pinarol commented Aug 30, 2019 •
edited
Loading

I ran Android UI tests(yarn test:e2e:android:local) locally by updating __device-tests__/helpers/caps.js > deviceName: 'Pixel 2 XL API 28' since that's the one available in my local.

They look fine:

Test Suites: 8 passed, 8 total
Tests:       21 passed, 21 total
Snapshots:   0 total
Time:        524.039s
Ran all test suites.
✨  Done in 581.71s.

@pinarol pinarol merged commit 8e3710a into develop Aug 30, 2019
@pinarol pinarol deleted the callstack/media-upload-props branch August 30, 2019 11:20
daniloercoli added a commit that referenced this pull request Sep 4, 2019
@daniloercoli
Merge branch 'develop' of https://github.com/wordpress-mobile/gutenbe…
71c73ff 
…rg-mobile into add/autosave-monitor

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (56 commits)
  Update gutenberg ref
  Update gutenberg ref
  Update gutenberg ref
  Update gutenberg ref
  Add support for group component (#1335)
  Update gutenberg hash
  fix error generating app name on sauce
  Bump eslint-utils from 1.3.1 to 1.4.2
  Update gutenberg ref
  Update release notes
  Update gutenberg ref to rnmobile/master HEAD
  disable paste tests
  MediaUpload and MediaPlaceholder unify props (#1310)
  Update gutenberg ref (#1340)
  Update gutenberg ref
  disable list tests and enable paste tests
  enable list tests
  enable list end tests
  enable block insertion tests
  enable heading tests
  ...

# Conflicts:
#	gutenberg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinarol pinarol pinarol approved these changes

@koke koke Awaiting requested review from koke

Assignees

@dratwas dratwas

Labels
Media
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dratwas @pinarol @koke @etoledom