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

Cover image block markup in the front-end #2902

Closed
samikeijonen opened this issue Oct 6, 2017 · 10 comments
Closed

Cover image block markup in the front-end #2902

samikeijonen opened this issue Oct 6, 2017 · 10 comments
Labels
[Feature] Blocks Overall functionality of blocks Good First Issue An issue that's suitable for someone looking to contribute for the first time

Comments

@samikeijonen
Copy link
Contributor

samikeijonen commented Oct 6, 2017

Markup:

<section class="wp-block-cover-image has-background-dim" style="background-image:url(http://wordpress-svn/src/wp-content/uploads/2017/09/abc.jpg)">
	<h2>Donate NowWe need you</h2>
</section>
  • Based on HTML5 specs <section> should not be used, should be <div> instead.
  • <h2> should not be hardcoded, it may be wrong heading level for correct heading hierarchy.

Quick note about CSS.

h2 {
		// ...
		font-size: 24pt;
		// ...
	}

	&.has-background-dim:before {
		background: rgba( black, 0.5 );
	}

Let theme handle font-size of the heading. At least it should not use pt. background should be background-color.

@karmatosed
Copy link
Member

I think having some styling is important. I also think we have to be cautious that a lot of themes handle fonts kind of badly. Maybe we can look at a % middle ground.

@karmatosed karmatosed added the [Feature] Blocks Overall functionality of blocks label Oct 9, 2017
@samikeijonen
Copy link
Contributor Author

Heading would also need a class for styling if <h2> is not hardcoded. Styling like this would be another option:

wp-block-cover-image h2,
wp-block-cover-image h3,
wp-block-cover-image h4,
wp-block-cover-image h5,
wp-block-cover-image h6 {

}

I also think we have to be cautious that a lot of themes handle fonts kind of badly

So does the Gutenberg :)

@mtias mtias added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Oct 10, 2017
@mtias
Copy link
Member

mtias commented Oct 10, 2017

I wonder if we should not use a heading at all here.

@afercia
Copy link
Contributor

afercia commented Oct 19, 2017

I wonder if we should not use a heading at all here.

Good question 🙂 If I'm not wrong, the <h2> is also hardcoded and there's no way to change its level. That's an assumption, because a h2 might not be always. appropriate.

Ideally, users should be allowed to use the markup they want in this block.

About ths <section> element: it's not appropriate in this case. Even a quick reading of the spec clarifies it pretty well: https://www.w3.org/TR/html51/sections.html#the-section-element

The section element represents a generic section of a document or application. A section, in this context, is a thematic grouping of content.

I don't see how a background image and a heading can be considered a thematic grouping of content.

@afercia
Copy link
Contributor

afercia commented Oct 19, 2017

@mtias maybe worth checking if there are other cases were hardcoded markup gets output. I no case WordPress and Gutenberg should output markup that can't be filtered. Want me to open a new issue?

@samikeijonen
Copy link
Contributor Author

<h2> is hardcoded at the moment. If we allow any heading level here, as we should, could we add class for styling:

Markup suggestion, no hardcoded <h2>:

<div class="wp-block-cover-image has-background-dim" style="background-image:url(http://wordpress-svn/src/wp-content/uploads/2017/09/abc.jpg)">
	<h2 class="wp-block-cover-image-title">Donate Now – We need you</h2>
</div>

Or without heading at all:

<div class="wp-block-cover-image has-background-dim" style="background-image:url(http://wordpress-svn/src/wp-content/uploads/2017/09/abc.jpg)">
	<p class="wp-block-cover-image-text">Donate Now – We need you</p>
</div>

@samikeijonen
Copy link
Contributor Author

I think button-like-link is coming to this block also.

@jeffpaul
Copy link
Member

This ticket was mentioned in Slack in #core-editor by jeffpaul. View the logs.

@samikeijonen
Copy link
Contributor Author

@jeffpaul: PR #3444 is ready for merge.

@samikeijonen
Copy link
Contributor Author

This is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Good First Issue An issue that's suitable for someone looking to contribute for the first time
Projects
None yet
Development

No branches or pull requests

5 participants