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

adds default alt values to inline images and the image block #10960

Closed

Conversation

antpb
Copy link
Contributor

@antpb antpb commented Oct 23, 2018

Sets default alt values to fix #1520
Previous conversation can be found here: #10482

How to test:
create image block
add image with no alt value
inspect image to see that alt value is now set as a readable string "This image has an empty alt attribute, its file name is "FILENAME""

save( { id, url, alt, width } ) {
save( { id, url, alt, width, filename } ) {
if ( ! alt ) {
alt = sprintf( __( 'This image has an empty alt attribute; its file name is "%s"' ), filename );
Copy link
Member

Choose a reason for hiding this comment

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

The issue #1520 is only about adding this text to images in editing context. The save() method is what gets saved into the database and is visible on the front end. So this change seems wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change this to only be the edit context. I set the alt if it is empty on the save method because from my understanding, screen readers will try and read the full image url if there is no alt attribute defined.

Copy link
Member

Choose a reason for hiding this comment

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

/cc @iseulde in case this would have to be included in format API PR.

Copy link
Member

Choose a reason for hiding this comment

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

Related chat on Slack where this has been discussed: https://wordpress.slack.com/archives/C02SX62S6/p1540406745000100

tldr;: empty alt texts are fine, suggested change should only apply in editing context.

@antpb
Copy link
Contributor Author

antpb commented Oct 26, 2018

@ocean90 Thanks for the catch. :) I've updated the PR to only apply the default alt text in the editing context if no alt is defined. I also tested adding alt text to the inspector settings and it updates from the default to the new inspector alt value as expected.

If you have any recommendations on how to better optimize this approach, I'd really appreciate it.

chrisvanpatten and others added 21 commits October 25, 2018 21:31
* Deprecate PanelColor components

* Update deprecation version numbers

* Fix a test broken during the rebase
… and Heading blocks for mobile, tryin gto stay closer to the web version. (WordPress#10690)
* Fix autocomplete cursor key navigation

* Avoid running onClickOutside logic for other non-onClickOutside events

* Add e2e tests for keyboard interaction with link container
* Update REST Search controller to use 'wp/v2' namespace

* Docs: Update REST API url
* Polish Save Draft/Switch to Draft area
* Use a Button for Trash button
* Polish the toggles.
…s#10541)

* Add basic responsiveness.

* Refactor columns metrics. Level the playing field in editor and frontend.

* Add space between colums.

Fixes WordPress#7818.
Fixes WordPress#6048.

* Remove "beta" designation from columns block.

* Columns block: Fix column width when editing

* Column block: Improve padding for the first and last item in a row

* Tests: Rename Columns block name also in e2e tests
## Description
This commit renames cover image block into cover and adds video support o cover.
The approach used to rename the block is the same used for the move of core/text to core/paragraph.
A change in api/parser.js. This approach may be seen as a temporary solution until a final approach to this use case exists, and we feel comfortable in exposing an API for this.

Transformations were updated, and a video transform was added.

## How has this been tested?
Verify the cover block still works as expected, containing the same functionality the existed in the cover image.
Verify it is possible to set video as background in the cover block.
* Extract isBlobURL into blob package and update usage

Co-authored-by: imath <[email protected]>

* Add ability to insert an image in image block using a url

Co-authored-by: imath <[email protected]>

* Position url input underneath upload file buttons and apply margin

Co-authored-by: imath <[email protected]>

* Refactor styles to avoid use of element selector

Co-authored-by: imath <[email protected]>

* Ensure form elements have bottom margin to separate them on tiny screens

Co-authored-by: imath <[email protected]>

* Change type of block used in e2e test to a separator

As mentioned in the comment above, this test will not work for blocks that
contain an input. The image block now contains a url input field causing
the test to fail.

Co-authored-by: imath <[email protected]>

* Fix invalid reference to this.isBlobURL

* Add expandable section for url form on media placeholder

* Change icon for media placeholder button to left/right chevron

* Update snapshots

* Try: expanding url input based on whether text is entered

* Try: use link popver for Insert from URL in media placeholder

* Implement URLPopover in MediaPlaceholder

* Use consistent declaration of defaults for props

* Use a generalized selector for buttons in the media placeholder

* Improve copy in URL input placeholder

* Make behaviour when selecting a differnent url the same as other media blocks

* Update snapshot tests

* Revert "Change type of block used in e2e test to a separator"

This reverts commit 6c8546c.

* Use onClose over onClickOutside (onClose implements onClickOutside)

* Rename functions

* Address code review feedback

* Update changelog for blob package

* Updated changelog for editor package
* First pass at adding 'Advanced Panels' to Options modal

Allows the user to enable or disable 'Advanced Panels' (AKA Meta Boxes)
via the Options modal.
* Catch a Text Selection Exception in Safari; fixes WordPress#8254

* Improve the inline explanation of why we need a try/catch block
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
* Update clipboard dependency to at least 2.0.1

This upgrades the clipboard dependency to at least 2.0.1, which resolves a possible conflict with the browser global Clipboard.

See https://github.com/zenorocha/clipboard.js/releases for release notes

* take the update to package-lock offered by npm i

* take another update to package-lock from a second run

* package-lock after removing lock entirely and rerunning npm i

* Update package-lock.json

* Update package-lock.json

* Update package-lock.json
swissspidy and others added 23 commits October 25, 2018 21:31
* Make preview placeholder text translatable

* Escape i18n strings since they are directly added to markup

* Use renderToString() and decodeEntities()

* Address feedback
* Use props instead of derived state to render the component.

There was a bug that happened when:

- a post was scheduled for a future date from now
- then the data changed to a past date from now

At this point, the post couldn't be published directly
by clicking the "Publish" button. This component
would show the post-publish panel with a "Post scheduled" status.

* Test: check that pre-publish panel is shown

* Test: check that post-publish panel is shown if post is published

* Test: check that post-publish panel is shown if post is scheduled

* Test: isScheduled is true, but isBeingScheduled false

Although the post status is 'future', the data is before now,
so we should show the pre-publish panel with the Publish button.

* Fix isSaving logic

* Update snapshots

* Test: check that spinner is shown if post is being saved

* Remove unused prop

* Name spinner component so snapshot test is more semantic

* Fix typo

* Update snapshot
…ress#10861)

Add default title and instructions labels to MediaPlaceholder based on allowed types.
* Testing: Add missing e2e tests for Plugins API

* Make all plugin API e2e test independent
…#10652)

* Add an eslint rule to use cross-environment SVG primitives

* Extend the list of forbidden elements
Uses an existing react eslint rule to perform validation.
Adds new components to make the existing icons mobile app friendly.

* Change the way the default icon for block is set

* Ensure that default icon goes through the process of width and height normalization in BlockIcon
Props to @jasmussen for helping to catch it.
It overrides the default so the bold doesn’t appear
* Return 0 if text is empty

* return 0 with empty stirng

* return 0 for enpty string

* small typo

* Add message to changelog
* Improve the style variation control aria-label.

* Further simplify the aria-label to just use the style label or name.
…ress#11040)

* Improve handling of centered 1-col galleries with small images

If you insert a gallery that has a single column, and multiple smallish images, then center it, the images inside do not appear centered.

This is technically correct, since the _gallery itself_ is centered, but because it's full width and because the images might vary in widths, this doesn't appear to make any difference to the content inside. So although "correct", it's not a good user experience.

This PR simply centers the content also.

* Remove editor specific alignments.
* Date: Mark getSettings as experimental

* Update all references to __experimentalGetSettings

* Bump deprecated version

* Add change to changelog

* Remove deprecated from 4.2

Co-Authored-By: ajitbohra <[email protected]>

* Marks as unreleased

Co-Authored-By: ajitbohra <[email protected]>
* Fix dynamic blocks not rendering in the frontend

* Use variables for hook priorities

* Test that dynamic blocks are rendered when meta boxes use the excerpt

* Fix autop hook order

* Fix linting

* Fix scoping issues

* Add an e2e test for the autop issue

* copy/paste issues

* Trying to fix travis

* Trim content to avoid small differences between themes
This removes the conditional branching based on the existence of the `get_compact_response_links()` method on the `$server` object, as that method was introduced with WordPress 4.5, and Gutenberg requires WordPress 4.9.8 or later.

Also, it turns the indirect and slow call through `call_user_func()` into a direct method call.

Note: The call would actually be more correct as a static call (`$server::get_compact_response_links( $response)`), but this notation is PHP 5.3+ only.
The end tag pattern is currently generated like this:
```
$end_tag_pattern = '/<!--\s+\/wp:' . str_replace( '/', '\/', preg_quote( $block_name ) ) . '\s+-->/';
```

Instead of separately using a string replace to replace `/` with `\/`, it is preferable to use the second argument `$delimiter` that the `preg_quote()` function provides.

See http://php.net/manual/en/function.preg-quote.php
@gziolo gziolo requested a review from a team October 26, 2018 08:07
@@ -512,6 +541,45 @@ class ImageEdit extends Component {
}
}
/* eslint-enable no-lonely-if */
if ( ! alt ) {
return (
Copy link
Member

@aduth aduth Oct 26, 2018

Choose a reason for hiding this comment

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

There's a ton of code duplication here. What's actually different? Can we refactor to limit the variation of this specific condition? i.e. if it's to return imgNoAlt instead of img, just change that in the existing return value:

{ alt ? img : imgNoAlt }

This will prove to be a boon to future maintainers who don't then need to update in two places to make changes to e.g. resizable box (or, more likely, forget to update altogether).

@@ -58,7 +58,9 @@ const LINK_DESTINATION_CUSTOM = 'custom';
const ALLOWED_MEDIA_TYPES = [ 'image' ];

export const pickRelevantMediaFiles = ( image ) => {
return pick( image, [ 'alt', 'id', 'link', 'url', 'caption' ] );
return {
...pick( image, [ 'alt', 'id', 'link', 'url', 'caption' ] ),
Copy link
Member

Choose a reason for hiding this comment

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

Why spread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually reverted this change prior to the nasty rebase attempt. The spread made sense in a prior iteration where I was changing the alt value. Thanks for calling out. :)

@aduth
Copy link
Member

aduth commented Oct 26, 2018

You're going to have an easier time against merge conflicts if you rebase and keep to a single commit.

https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase

@antpb
Copy link
Contributor Author

antpb commented Oct 29, 2018

:( I made this PR really messy with that last rebase attempt. Sorry. I've added a new, more final PR that should be easier to review/merge: #11218

@antpb antpb closed this Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Images alt attribute in an editing context