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

Convert quote to / from pullquote #10683

Merged
merged 2 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@nosolosw
Member

nosolosw commented Oct 17, 2018

Addresses #10548

Adds a transformation to convert quote to pullquote and viceversa.

@nosolosw nosolosw requested a review from WordPress/gutenberg-core Oct 17, 2018

@nosolosw nosolosw self-assigned this Oct 17, 2018

@nosolosw nosolosw added this to the 4.1 - UI freeze milestone Oct 17, 2018

@nosolosw nosolosw requested a review from mcsf Oct 17, 2018

@tofumatt

E2E tests for these types of things (just a toMatchSnapshot() should work) are always great, sorry to harp on about them!

(Otherwise code looks good to me.)

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Oct 17, 2018

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 17, 2018

@tofumatt how do I prepare a snapshot? I haven't done it before.

I wouldn't ask if we weren't counting the minutes for the UI freeze because I can google, but 🤷‍♂️

@talldan

This comment has been minimized.

Contributor

talldan commented Oct 17, 2018

@nosolosw There's an example here of using snapshots in e2e tests:
https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/links.test.js#L55

Typically, when you run jest (npm run test-e2e or npm run test-e2e:watch) the snapshot files will be created automatically the first time. If the snapshots need to be updated, jest usually tells you in watch mode and you can use the u key to update them.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 17, 2018

No worries! jest (npm run test-e2e:watch) will generate them for you for all new tests. Just write:

expect( await getEditedPostContent() ).toMatchSnapshot();

at the end of your test and look for the new snapshot file generated. Ensure the HTML looks like what you expect, commit it, and then you're good. Once it's generated, if the result ever changes jest will yell at you 😄

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 17, 2018

@talldan and I are secretly the same person.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 17, 2018

That's cool! Added e2e test in 45f89a9 Will merge after lunch if CI doesn't yell at me.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 17, 2018

And if any of your avatars ( @tofumatt / @talldan ) approves the PR :)

@tofumatt

Very nice (tests are the best)! 🚢

@nosolosw nosolosw merged commit f2a5f1f into master Oct 17, 2018

2 checks passed

codecov/project 49.53% (-0.01%) compared to c7dc7a9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nosolosw nosolosw deleted the add/transform-quote-pullquote branch Oct 17, 2018

@paaljoachim

This comment has been minimized.

paaljoachim commented Oct 22, 2018

The bottom block title goes below the edge of the blocks area.

screen shot 2018-10-22 at 18 43 28

z-index-issue

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 22, 2018

@paaljoachim I'm not able to repro ( the popover correctly changes up/down when there is no space ). Tried in both mobile viewports and desktop, scrolling both with the keyboard (arrows) and the mouse (grabbing the scrollbar). Could you provide more details?

@paaljoachim

This comment has been minimized.

paaljoachim commented Oct 22, 2018

Hey @nosolosw

I am trying various things to replicate it, and I am noticing it is difficult to get it the exact same. It seems to happen in certain conditions that I was able to replicate at first but after trying a few variants I am not able to do so again. So I will we will just let it go for now and just call it a gremlin in the system...:) If it comes up again then you atleast know about it.

@nosolosw

This comment has been minimized.

Member

nosolosw commented Oct 23, 2018

@paaljoachim thanks for your testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment