-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add aside wrapper to pullquote #9599
Conversation
Sorry but the We should definitely avoid a pullquote is listed and announced as a landmark region by assistive technologies. In the editor, there should be just the main landmark regions defined by Gutenberg. In the front-end, the usage of an https://www.w3.org/TR/html52/sections.html#the-aside-element
Note the same issue as discussed time ago for the core widgets (can't find the Trac ticket right now), as in some themes each widget in a sidebar uses an |
I think in this case a "figure" might be more semantic given the intention. |
Whatever, as long as it's not an aside 🙂 |
Thanks for the change. I wonder why Just to document what was happening with the |
Should be added to front-end too, yes. |
Noting that we might need to account for migration if this would invalidate previously-saved pullquote blocks. |
Great point. This is what happens now; this is a pullquote created in master: This is what I saw when checking out this branch on the same post: This is what happens when I pressed "Convert to blocks": So looks like citation is lost, but I feel like that's tracked somewhere? Would appreciate input on how to provide a migration path if anyone has time. |
@jasmussen You could just use the deprecated block API in this case. The Paragraph block has some examples of using it. https://wordpress.org/gutenberg/handbook/block-api/deprecated-blocks/ |
ca94e93
to
10feb74
Compare
5f7ceb9
to
17e4292
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased this PR, and had a final look at it. I think the changes look good 👍
Just for reference:
So we are definitely going against the spec on this. That said, I understand the reasoning, so I am fine with using a |
This is an alternative, per discussion, to #8821. It adds some semantic value to the pullquote, indicating that it is intended to be separate content. Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it: - Let's add a transformation from Quote to Pullquote and back - For some reason, the alignment values are not output in the markup in the editor, only on the frontend, which means the max-width we apply does not affect the content. I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?
17e4292
to
fc1eac0
Compare
); | ||
}, | ||
|
||
deprecated: [ { | ||
attributes: { | ||
...blockAttributes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Spread operator is not necessary here (and has a small performance cost), vs. just assigning directly as attributes: blockAttributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the blockAttributes
variable was just called attributes
, you could make it even shorter:
attributes,
The demo content should have been updated here. While the deprecation migration saves it from being flagged as invalid, it does cause the demo post to trigger an autosave after delay.
|
This is an alternative, per discussion, to #8821.
It adds some semantic value to the pullquote, indicating that it is intended to be separate content.
Note that we should consider doing a few other enhancements to the pullquote, if we intend to keep it:
I could use help with both of these. I also imagine the deprecation handler needs a little extra now. @gziolo do you have bandwidth?
Screenshot showing the max width not being applied (note, this is not a new regression, this is in 3.7):