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

Fix: Multiple block warning min height. #8006

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jul 17, 2018

Description

The min-height style, for multiple block warnings, was not correct and the design was not right.

How has this been tested?

I pasted the following string in the code editor:

<!-- wp:subhead -->
<p class="wp-block-subhead">Subhead 1</p>
<!-- /wp:subhead -->

<!-- wp:subhead -->
<p class="wp-block-subhead">Subhead 2</p>
<!-- /wp:subhead -->

I checked the design of the multiple block warning looks good.

Screenshots

Before:
image

After:

image

@jorgefilipecosta jorgefilipecosta force-pushed the fix/multiple-block-warning-min-heigth branch from 15a6a99 to 503a96c Compare August 1, 2018 16:43
@jasmussen
Copy link
Contributor

Interesting, I'm not seeing your "after" image when I try this branch. I'm seeing this:

branch

Which looks good to me, by the way, but wanted to note.

Also, I'm seeing a JS error when trying to save after the fact. Am I doing something wrong?

nonce.js:18 Uncaught (in promise) TypeError: Cannot read property 'addAction' of undefined
    at nonce.js:18
    at next (index.js:93)
    at root-url.js:33
    at namespaceAndEndpointMiddleware (namespace-endpoint.js:21)
    at root-url.js:11
    at next (index.js:93)
    at preloading.js:43
    at next (index.js:93)
    at apiFetch (index.js:96)
    at _callee$ (posts.js:130)

See also #7741

@jorgefilipecosta jorgefilipecosta force-pushed the fix/multiple-block-warning-min-heigth branch from 503a96c to f34c1ef Compare August 2, 2018 15:17
@jorgefilipecosta
Copy link
Member Author

Sorry, @jasmussen this PR was old and things change very fast. This PR was rebased with auto merge so I did not retest everything wrongly assuming it was still valid.
I think the new design can still benefit from an update to the min-height, so I updated this PR and the before and after images.
Regarding the error, I'm not being able to reproduce it this was rebased again before my tests could you please check if the error is still happening?
Thank you a lot for the review!

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Now seeing this:

screen shot 2018-08-02 at 19 17 44

This looks good, nice work! Also, the JS errors appear to be related to my dev env. Probably cache or something. Nice work!

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.

2 participants