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

Add Block: Image #310

Closed
mtias opened this issue Mar 23, 2017 · 19 comments
Closed

Add Block: Image #310

mtias opened this issue Mar 23, 2017 · 19 comments
Labels
[Feature] Blocks Overall functionality of blocks [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@mtias
Copy link
Member

mtias commented Mar 23, 2017

Attributes

  • None, Left, Right, Center, Full Bleed.
  • Caption.
  • Link.
  • Edit.

Markup

<!-- wp:image -->
<img src="/" />
<!-- /wp -->
<!-- wp:image -->
<figure>
    <img src="/" />
</figure>
<!-- /wp -->

With caption:

<!-- wp:image -->
<figure>
    <img src="/" />
    <figcaption>A picture is worth a thousand words.</figcaption>
</figure>
<!-- /wp -->

States (see all)

image

image

image

@mtias mtias added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take labels Mar 23, 2017
@youknowriad
Copy link
Contributor

youknowriad commented Mar 23, 2017

About the Image Alignment, we had a discussion with @jasmussen and it looks like these alignments could be relevant to all blocks (float alignment), Should we consider adding them in a global way or a block should add them explicitly?

Edit:

I also think the image can have a width attribute if it's resized

@aduth
Copy link
Member

aduth commented Mar 23, 2017

@youknowriad This was one of the thoughts with registerControl in #302 was that we could define generalized controls like wp/alignright which simply setAttributes( { align: 'right' } ) that the block implementation is expected to account for.

@mtias
Copy link
Member Author

mtias commented Mar 23, 2017

I imagine we would have generalized controls, but a block needs to opt in specifically, because alignment can mean text-align, floating, or some flex-magic for other blocks.

@joyously
Copy link

Can you use a different term for "full bleed"? As a user, I have no idea what that is. It doesn't sound like a type of alignment.

@jasmussen
Copy link
Contributor

Can you use a different term for "full bleed"? As a user, I have no idea what that is. It doesn't sound like a type of alignment.

"Full-width"?

@joyously
Copy link

"Full-width"?

Is that what "full bleed" is? How is a width an alignment? Kind of like "justify" is with text?

@jasmussen
Copy link
Contributor

Kind of like "justify" is with text?

Basically.

@joyously
Copy link

I say that's not alignment. I would leave out that "option" entirely from core. Let plugins add it if they want. The existing alignments cover the bases: none, left, right, center.

@youknowriad
Copy link
Contributor

I would leave out that "option" entirely from core.

I think the "Alignment" name here is probably not suited because it creates ambiguity with text alignment. The "Alignment" attributes here refer to block "positioning" instead of "alignment".

To get a sense of what these buttons do, you can try on one of the prototypes (here for example https://wordpress.github.io/gutenberg/tinymce-per-block/)
screen shot 2017-03-23 at 16 51 27

And I think these are really important to have to compose the different blocks together. I think they can be added automatically to all blocks. Even Text blocks can be "floated" or "positioned" left/right etc...

@joyously
Copy link

I think the "Alignment" name here is probably not suited because it creates ambiguity with text alignment.

Specifically here, on the image block, is what I'm talking about. "Alignment" is perfectly suited and is already used in core for images. What is not suited is the extra one that isn't really alignment. It's more like "size". And it wouldn't really apply to other types of blocks. Images(videos) are the only thing that is really a monolithic block where the full-bleed would apply. Simpler blocks have formatted text where text alignment comes into play, but full-bleed does not make sense for the simple block. And the same goes for the more complicated blocks like a map or gallery. The left, right, center would apply to the block, but full-bleed makes no sense for a block with multiple parts to it.
If anything is missing for Alignment, it would be one called Clear.

@jasmussen
Copy link
Contributor

I think we may be over emphasizing on labels, which we can change right up until the last minute, as unless you use a screen reader or rest your cursor upon the button, you won't see the label at all.

What we have to work with here — and yes these labels and terms are open to suggestion — are block level layout alignments and text alignments.

For text, left, center, right are alignments you'll use on paragraphs.

For images, left, center, right are alignments WordPress supports currently, and these are very different from text alignments as left and right lets text float around it. The key difference is they can be additive: you could theoretically right align text inside a box that was floated left, not that we'd surface buttons for these.

But a full-width button (agree, full-bleed is a anachronistic print term that doesn't fit here) to let you make images, video, galleries, maps, embeds, custom fields, could all benefit from new layout features that haven't existed in WordPress to date.

full-bleed does not make sense for the simple block

Agree, there are many blocks for which full-width (née full-bleed) makes no sense. We've discussed blocks and what features they should have also in #16.

@mtias
Copy link
Member Author

mtias commented Mar 24, 2017

I removed the "alignment" label, it's inconsequential since we don't show it.

@joyously
Copy link

I think we may be over emphasizing on labels

Well, not really. I asked about the label used so I could understand what we were talking about. I thought this was about what attributes go on what blocks as well as what controls show up for those blocks. To that end, I still think you need Clear to round out the alignment set. And Clear would be additive to Left, Right, Center. And image width attribute is missing on this block (images).

I still think that max-width is not appropriate in the same control box as alignment is. It belongs in a higher level "layout" context that takes window size into account, so while it would be related to the block, it is really the default value for blocks, and not something that needs to be stored as an attribute. Your prototype is deceiving because you limit the width and then let it be the default when you click the button, but that is not how it would be on the front end.

Discussing this detail brings up the point that even Left, Right alignment could benefit from being conditional, meaning that Right alignment for big windows and None for small windows is a common pattern. What I would like to avoid is micro-management. This is not desktop publishing. It is the web. The browser is really good at presentation if you don't give it too many instructions. Users notoriously design for one size and don't think about the rest. That is the theme's job. I am in favor of having fewer controls, rather than more, for the simple reason that users will overuse them. And that's why I advocate that core limits what it supplies in the editor. Let plugins add the fancy blocks, and let the themes do the "layout". Just do the basics well.

For text, left, center, right are alignments you'll use on paragraphs.

And why leave out justify for text? (even though it's not that great to use) Actually, justify can be a really good way to align inline-blocks, like for a gallery, so it's not just about text. But that should be up to the theme, and not the user. In my mind, that brings up the fact that images are actually inline elements by default, so text alignment applies (and affects how the alt text is shown).

@jasmussen
Copy link
Contributor

We should expect to do many tweaks as we enter the plugin phase and get a feel for how users will use it.

And why leave out justify for text?

This is a past decision that this project should not overturn without good reason.

@aduth
Copy link
Member

aduth commented Apr 6, 2017

Can you clarify the two markup variants, i.e. with and without figure. Is this what we expect to be able to parse? In what form do we save?

@westonruter
Copy link
Member

@aduth I'd like to highlight concurrent work being done on the core Image Widget, which is nearing merge into trunk:

PHP: https://github.com/xwp/wp-core-media-widgets/blob/master/wp-includes/widgets/class-wp-widget-image.php
JS: https://github.com/xwp/wp-core-media-widgets/blob/master/wp-admin/js/widgets/media-image-widget.js

You'll note that the widget is able to represent all of the properties that the media library returns from the select frame and the image details frame. The naming of the properties has undergone some normalization for storage in the widget instance, so I want to make sure that we are in align-ment (pun intended) on naming conventions to minimize differences between what an image block looks like and what a image widget looks like, from a data perspective.

@aduth
Copy link
Member

aduth commented Apr 6, 2017

@westonruter Very relevant given #379, thanks for the pointer! I agree it's sensible to keep property names consistent as much as possible. I'll seek to rename src to url in #379 (I'd contemplated it anyways). Your link also gives a more complete sense of the properties we'll likely need/want to support.

@joyously
Copy link

joyously commented Apr 6, 2017

Can you clarify the two markup variants, i.e. with and without figure. Is this what we expect to be able to parse? In what form do we save?

I vote for only having figure when there is a caption. Otherwise it could throw existing styling off.

aduth added a commit that referenced this issue Apr 7, 2017
@jasmussen jasmussen added this to the Prototype Parity milestone Apr 20, 2017
@mtias mtias removed this from the Prototype Parity milestone Apr 27, 2017
@mtias mtias added [Feature] Blocks Overall functionality of blocks and removed Framework Issues related to broader framework topics, especially as it relates to javascript labels May 3, 2017
@jasmussen
Copy link
Contributor

I'm going to close this for now as fixed, and instead refer to specific enhancement tickets, #600 and #546

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 [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

6 participants