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

Update/cover image block #12405

Closed
wants to merge 10 commits into from

Conversation

mrmadhat
Copy link
Contributor

Description

Changes paragraph text to heading with a selectable heading level see #10746

How has this been tested?

I have viewed the changes in browser

Screenshots

image

Types of changes

Updates Cover block

Checklist:

  • [x ] My code is tested.
  • [x ] My code follows the WordPress code style. <!-- Check code: npm run lint, Guidelines:

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @mrmadhat, thank you for your contribution. Previously we used headings in the cover image, but a change was applied and we intentionally changed to a paragraph. That can still be observable in the deprecations code.
I think this chnage was related to the semantics of the markup.
The change was applied in commit 77cd75d. @samikeijonen would you be able to provide some context on the reason of this change?

Edited: the related PR is #3444 and the related issue is #2902.

@mrmadhat
Copy link
Contributor Author

In the issue related to this pull request #10746 I've explained the reasoning.

The cover image block specifies in the description "Add a full-width image, and layer text over it — great for headers." However, the text outputs in a paragraph tag and not a header.

The placeholder text also states "enter title..."

The block is misleading, it tells the user the text that they are adding to this block will be treated as a heading but it currently is not.

For users using covers as heading this will affect accessibility (Cover blocks will not make up part of the document outline) and and seo.

@ajitbohra ajitbohra added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Nov 29, 2018
@samikeijonen
Copy link
Contributor

Originally heading level was hard coded h2 which may or may not be correct heading level. Therefore we switched to paragraph.

I'd be open to idea if the heading level can be changed though.

@gziolo
Copy link
Member

gziolo commented Feb 5, 2019

@mrmadhat and @jorgefilipecosta where are we with this PR? What is missing to land this in the upcoming Gutenberg 5.1 release?

@jorgefilipecosta
Copy link
Member

From my part, I don't see blockers given that @samikeijonen the original creator of #2902 is open to this idea.
@afercia would it be possible to share some thoughts of this change regarding the a11y point of view?

@mapk regarding the UX do you think this change is valuable? Also, what are your thoughts regarding the usage of nested blocks on the cover block? If we think the user experience using nested blocks is preferable we should probably go directly there.

@mapk
Copy link
Contributor

mapk commented Feb 6, 2019

The argument that hardcoding a heading (ie. H2) may cause an incorrect heading hierarchy makes sense. I see the reasoning for changing it to a p. However, I do believe a common usage of the cover block is for headings as outlined in the documentation.

That being said, I'd like to add back the heading capability to the cover block, but allow the heading level to be user selected. This appears to align best with the concept of nested blocks at this point as @jorgefilipecosta points out; a nested heading block inside the image block, to make up the Cover block. Let's go that direction.

@gziolo gziolo added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility labels Feb 6, 2019
@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

This appears to align best with the concept of nested blocks at this point as @jorgefilipecosta points out; a nested heading block inside the image block, to make up the Cover block. Let's go that direction.

I also share a similar view in regards to that. I also sense some accessibility challenges introduced by this proposal due to the fact that tag name would have to be updated in the sidebar. In general, taking the approach of nested blocks gives it much more flexibility in terms what kind of blocks could be nested (not only heading) but also would open room for further customizations inherited from individual blocks. I would vote to stop working the original issue and this PR and focus on converting Cover block to use nested blocks internally. I think @jorgefilipecosta was exploring that in the past. What blocked it from getting in WordPress 5.0 release?

@samikeijonen
Copy link
Contributor

I like the idea of nested blocks because it gives possibility to add Button block inside the Cover block also.

There must be issue about that already but I can't find it at the moment.

Let's still be careful with using headings. Users might pick heading just based on the size, not the correct hierarchy. Document outline info helps with this though.

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

Let's still be careful with using headings. Users might pick heading just based on the size, not the correct hierarchy. Document outline info helps with this though.

It might be also an interesting challenge to provide automatically adjusted heading type based on the surrounding content. I think this is something that could be explored on its own. The idea is that you add the heading without any size provided and it detects and sets the proper one depending on the previous headings.

@jorgefilipecosta
Copy link
Member

It might be also an interesting challenge to provide automatically adjusted heading type based on the surrounding content. I think this is something that could be explored on its own. The idea is that you add the heading without any size provided and it detects and sets the proper one depending on the previous headings.

I'm not sure if that's is possible, at most we can just discard some headings in a given context. For example, if the previous heading is h3 we know that the next heading cannot be h5. But it can be an h4 heading inside the previous h3, it can be another h3 heading sibling of the previous h3 or it can be another h2 heading sibling of the previous h3 parent. It is not possible to automatically know the user intent.
Another option is showing some visual feedback with the heading sections to improve the probability that the user is going to make the correct choice.

@jorgefilipecosta points out; a nested heading block inside the image block, to make up the Cover block. Let's go that direction.

I would vote to stop working the original issue and this PR and focus on converting Cover block to use nested blocks internally. I think @jorgefilipecosta was exploring that in the past. What blocked it from getting in WordPress 5.0 release?

There were two tries to get nesting in cover, and in one of the tries, it was decided nesting was not polished enough. On the second try, we extracted from that the PR the video options and some refactors to cover but decided the nesting part would not go as part of phase 1.

So as an action item I can try to revive the nesting mechanism in cover. We then can review it and if concluded nesting is ready for that block we advance in that direction if not we may take a new look into this PR.

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

I'm not sure if that's is possible, at most we can just discard some headings in a given context. For example, if the previous heading is h3 we know that the next heading cannot be h5. But it can be an h4 heading inside the previous h3, it can be another h3 heading sibling of the previous h3 or it can be another h2 heading sibling of the previous h3 parent. It is not possible to automatically know the user intent.

I'm sure it is possible to make some inform decisions based on the previous heading or the fact that this is the first heading in the document. There are three states really in such scenario:

  • the same level which would be the default
  • child level
  • one of the parent levels which is the most complex one

Anyway, this is something nice to have which could help to work on larger documents and doesn't differ that much from lists which are handled this way using identation levels.

@gziolo gziolo added the [Status] Not Implemented Issue/PR we will (likely) not implement. label Feb 6, 2019
@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

So as an action item I can try to revive the nesting mechanism in cover. We then can review it and if concluded nesting is ready for that block we advance in that direction if not we may take a new look into this PR.

@mrmadhat thank you for your work invested in this PR. We think that there is a more flexible approach which is going to solve the same issue which we want to revive given that the UI for nested blocks is getting better with each plugin release. It would be great to have you involved in the further work on this capability given your experience learned with this code exploration.

@gziolo gziolo closed this Feb 6, 2019
@mrmadhat
Copy link
Contributor Author

Thanks @gziolo I've been away for the past week and I'm just catching up now, I agree with what's been said and would like to be involved with the work on the nested blocks. I think I've found the main issue for this functionality (4900) but could someone confirm what the go to issue for this is?

@gziolo
Copy link
Member

gziolo commented Feb 11, 2019

#4900 is a bit different. This is meant to provide a general purpose container block which might be a way to build Cover block yourself. However, let me copy my comment from a different PR:
#10746 (comment)

I think that #10639 was the last try to allow to use a few block types inside Cover block. Looking at the code I can notice that @jorgefilipecosta was exploring how to allow the following blocks to be used on top of the cover image/video (https://github.com/WordPress/gutenberg/pull/10639/files#diff-04b4f9f3332a93f24a032e821240c800R37):

  • button
  • heading
  • paragraph

I can't find any open PRs, so you might want to team up with @jorgefilipecosta and figure out what would be next steps :)

@mapk
Copy link
Contributor

mapk commented Feb 12, 2019

The PR is now shifted to #13822. @mrmadhat, would love to have you involved there!

@gziolo
Copy link
Member

gziolo commented Feb 12, 2019

Yeah, I think I commented on another PR or issue @mrmadhat authored 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Status] Not Implemented Issue/PR we will (likely) not implement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants