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 missing CSS in the Classic Block #12441

Merged
merged 1 commit into from
Mar 18, 2019
Merged

Conversation

azaozz
Copy link
Contributor

@azaozz azaozz commented Nov 29, 2018

Makes image captions and wpviews (embeds, etc.) work again.

Tested in Chrome, Firefox, IE11.

@azaozz azaozz added the [Block] Classic Affects the Classic Editor Block label Nov 29, 2018
@azaozz azaozz added this to the 4.6 milestone Nov 29, 2018
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.

Not sure how to test this. The idea seems fine, but I don't actually see any regression in not including this.

This is this branch:

screenshot 2018-11-29 at 18 55 02

This is master:

screenshot 2018-11-29 at 18 55 34

@mtias mtias modified the milestones: 4.6, 4.7 Nov 29, 2018
@azaozz
Copy link
Contributor Author

azaozz commented Nov 29, 2018

Uh, sorry, should have added screenshots.

image-caption-before
Image with caption before: image is too large, doesn't get the max-width as the caption element doesn't have it. Also note the resize handle top/left (only part is visible). The image tag's width is also wrong in Twenty Nineteen.

image-caption-after
Image with caption after: proper max-width, proper tag width for the caption element (arguably, as it is "hard coded" to the block width when the post was edited) and all resize handles are visible.

embed-before
The wpview (embeds) get an transparent overlay so we can show a popup toolbar on click (like on images). The styling for it is missing, so it doesn't cover the embeds.

embed-after
Embeds after: the overlay is in place and the toolbar shows.

@jasmussen
Copy link
Contributor

Ah, thanks for the screenshots. Yes this seems like a good improvement.

@youknowriad youknowriad modified the milestones: 4.7, 4.8 Dec 3, 2018
@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@azaozz
Copy link
Contributor Author

azaozz commented Jan 25, 2019

Ughhh, mistakenly thought these were fixed already... They are not "nice to have improvements", they are regressions. Most are pretty serious like the missing CSS for the shim covering embeds in the Classic block preventing the floating toolbar from showing, and users from being able to edit the URLs. The missing CSS for the error messages when embedding fails make them look quite "ugly" :(

All of this functionality has been in the Classic Editor for years. I understand the styling was missed when bringing over the CSS for editor content, but it needs fixing the-sooner-the-better.

@azaozz
Copy link
Contributor Author

azaozz commented Jan 25, 2019

Hmm, I'm probably missing something but how is this error related to this pr?

waiting for selector ".media-modal li[aria-label="cad387e3-cb4b-4972-a243-260cbfd3e953"]" failed: timeout 30000ms exceeded

@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Feb 5, 2019
@gziolo gziolo requested a review from ellatrix February 5, 2019 14:29
@ellatrix ellatrix force-pushed the fix/classic-block-css branch from 3f080f7 to 926aaf3 Compare February 11, 2019 09:49
@gwwar
Copy link
Contributor

gwwar commented Mar 9, 2019

@azaozz Would it be possible to note testing steps, what is broken/expected, and some context on the changes being made here?

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@azaozz azaozz requested a review from chrisvanpatten as a code owner March 18, 2019 10:56
@ellatrix ellatrix force-pushed the fix/classic-block-css branch from 01fb026 to 7ab5b40 Compare March 18, 2019 11:08
@ellatrix
Copy link
Member

I rebased this PR and cleaned it up a bit.

Testing instructions:

  • paste a youtube URL in the classic block. Ensure styles look good: border + toolbar etc.
  • Add an image with caption. Ensure styles look good: border + toolbar etc.

@ellatrix
Copy link
Member

@azaozz Could you confirm everything still looks good to you after the clean up? :)

@ellatrix
Copy link
Member

Also test: you should not be able to play the embed after selecting the block inside the classic editor.

@youknowriad youknowriad merged commit a6c4db8 into master Mar 18, 2019
@youknowriad youknowriad deleted the fix/classic-block-css branch March 18, 2019 11:23
@youknowriad youknowriad added this to the 5.3 (Gutenberg) milestone Mar 18, 2019
@azaozz
Copy link
Contributor Author

azaozz commented Mar 18, 2019

@ellatrix yep, works quite well here too. Thanks for looking at this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Classic Affects the Classic Editor Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants