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.

Sign up for GitHub

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

Jump to bottom

Slideshow: Replace Buttons With A Tags #31175

Merged
merged 5 commits into from
Mar 4, 2019
Merged

Slideshow: Replace Buttons With A Tags #31175

merged 5 commits into from
Mar 4, 2019

Conversation

jeffersonrabb
Copy link
Contributor

Changes proposed in this Pull Request

This is one of two PRs that resolve the issue caused by WPCOM stripping out <button/> elements, which causes the Slideshow block to become invalid on save. In this one, the <button/> elements are replaced by <a/>s.

The alternate approach is here: #31174

Testing instructions

  • Check out this branch in local wp-calypso repo
  • From within the wp-calypso folder, generate new blocks code targeting your sandbox's blocks-staging directory. The SDK command should look like this: npm run sdk gutenberg client/gutenberg/extensions/presets/jetpack -- --output-dir={PATH-TO-SANDBOX}/wp-content/mu-plugins/jetpack/_inc/blocks-staging/
  • Manually upload blocks-staging directory to your sandbox.
  • From root of wp-calypso, start local Calypso: npm start.
  • Navigate to http://calypso.localhost:3000/
  • Create a post, then add Slideshow block.
  • Turn on Autoplay, so that you will see all three buttons (prev/next/pause)
  • Inspect the block and verify that the three buttons are now <a/> elements
  • Save the post and refresh the page. Verify that the block is not invalidated.

Fixes #31029

@jeffersonrabb
Replace Slideshow buttons with A tags. Hide inner text in links. Disa…
74ccfe4 
…ble ESLint rule related to anchors without HREFs.
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/slideshow-no-buttons

@jeffersonrabb jeffersonrabb self-assigned this Mar 3, 2019
@jeffersonrabb jeffersonrabb added this to the Jetpack: 7.1 milestone Mar 3, 2019
@jeffersonrabb jeffersonrabb mentioned this pull request Mar 3, 2019
Closed
@simison simison requested a review from a team March 4, 2019 08:35
@simison
Copy link
Member

simison commented Mar 4, 2019 •
edited
Loading

We did some testing and chatting with @aldavigdis and @sirreal around this (p1551653790248800-slack-jetpack-gutenberg):

  • a without href "feels wrong" and we'd have to disable a11y linter for the rule. https://github.com/evcohen/eslint-plugin-jsx-a11y/issues/181 does seem to indicate it's not that bad.
  • @aldavigdis suggested we add href="#id-to-slide" which would be semantically correct and work also without JS (e.g. RSS readers or emails).
    • That's some extra code+new feature I'd rather leave out from beta, but it's up to you to weight it
    • adding href later or switching div to a (or button would require deprecation rules anyway)
    • using a now would not require rewriting styles later on when possibly adding href.
  • a supports both enter + space navigation, which is a big plus vs div which supports only enter. Proper button should support both.
    • div would require onClick + some JS to handle pressing Space correctly

Personally, I'd be inclined to go via a route but I'll leave it to you. I'm not aware of a without href being worse than div here, because former gives us functioning keyboard navigation without extra code and opens up a nice way to adding href later on.
We all agree button would be much better but oh well! (p5TWut-gm-p2 #comment-914)

simison
simison approved these changes Mar 4, 2019
View reviewed changes
Copy link
Member

@simison simison left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tests well. 👍

button would still be ideal but can't have it because of wpcom wp_kses ....

@jeffersonrabb jeffersonrabb merged commit 5a56931 into master Mar 4, 2019
@jeffersonrabb jeffersonrabb deleted the fix/slideshow-no-buttons branch March 4, 2019 20:40
jeherve pushed a commit that referenced this pull request Mar 4, 2019
@jeffersonrabb @jeherve
Slideshow: Replace Buttons With A Tags (#31175)
d1f401c 
* Replace Slideshow buttons with A tags. Hide inner text in links. Disable ESLint rule related to anchors without HREFs.

* Remove text from PREV/NEXT/PAUSE buttons, to avoid block invalidation after language change.

* Reposition bullets element below buttons (no visible change).

* Remove localization of text for initial autoplay Pause button state. All a11y label localization will be addressed as a later block of work.

* Removing unneeded CSS now that buttons have no inner content.
@jeherve
Copy link
Member

jeherve commented Mar 4, 2019

Cherry-picked to the release branch in d1f401c

jeherve added a commit that referenced this pull request Mar 5, 2019
@jeherve @sirreal
@enejb @jeffersonrabb @roccotripaldi @thomasguillot @simison @mmtr @gititon
Merge pull request #31045 from Automattic/update/jetpack-blocks-71
8c4cb5e 
* Implement image filters for tiled gallery block

* Jetpack Blocks: move a new set of blocks in production

See https://github.com/Automattic/jetpack/milestone/119

* Jetpack Blocks: tag new version of the blocks for the 7.1 release

* Contact Info: Fix/contact info fix sidebar (#31042)

* Contact Info Block: Remove the Sidebar for google maps link

* bug fix: use className instead of class

* Additional keywords for the Slideshow block. (#31038)

* Improvement to Autoplay delay label. (#31039)

* Slideshow: Add New Uploads At End (#31046)

* Add newly uploaded images to the end of the slideshow. Jump to the position of the new upload so user sees upload status.

* Alternate approach using comparison of image array length in componentDidUpdate to determine position.

* Contact Info: Add the ability to add the buisness hours block to the contact info (#31056)

* Contact Info Block: Improve email validation (#31033)

* Contact Info Block: Improve the email validation

By using the emailValidator that is use in more places we can provide better matching and of email addresses.

* bug fix. take into account any number of punctuation.

* Business hours: show 'closed' when data is incomplete for a given interval (#31083)

* Business hours
If data is incomplete for a given interval, display 'Closed' or null;

* adjusting logic - thanks @lezama

* further improved time validation logic

* Contact Info Block: Split phone numbers into prefix and phone number (#31026)

* Split phone numbers into prefix and phone number

This allows us to be not so strict with what a phone number is and still offer a way to prefix. things.

* Fix typos and simplify selection of first character.

* WordAds block: Update icon (#31106)

* Business Hours: Update editor styles for consistency with UI elements

* Business Hours Block: add search keywords (#31090)

* Business Hours Block:

add search keywords

* updating terms after feedback from co-workers

* Related Posts: update the message displayed when no related posts (#31126)

Discussion: p8oabR-kb-p2 #comment-2590

* Tiled Gallery: Remove i18n strings in save (#31119)

* Add deprecated declaration based on #30724

* Remove localized aria-label from save

* Form block: fix typo (#31139)

* Mailchimp: Placeholder Buttons (#31089)

* Rework of button UI hierarchy in unconnected placeholder.

* Change to Connection button styling.

* Add isBorderless to Re-check button.

* Revised button attributes for placeholder.

* Further revisions to button styles.

* Mailchimp: Form Element Spacing (#31142)

* Standardized form element spacing in the editor.

* Use rem for form element spacing.

* Business hours block: use blockIcon in placeholder (#31143)

* Fix: business hours validation (#31122)

* When saving a business hours block, don't save any data that hinges on localization.

* defaultLocalization should not be translated.

* After a conversation with @sirreal - we've decided on this approach:

drop the save component altogether, because we use server-side rendering for the block
have a <DayEdit /> and <DayPreview /> component for client side rendering, which are only rendered once we've fetched localization

* Default days should remain localized.

* Tiled gallery block: camel case product name (#31184)

* VideoPress: Disable re-usable block feature for video blocks (#31186)

* Change contact info icon to account box (#31190)

* Deselect image on filter change

* Update filter icons

via #30853 (comment)

* Slideshow: Replace Buttons With A Tags (#31175)

* Replace Slideshow buttons with A tags. Hide inner text in links. Disable ESLint rule related to anchors without HREFs.

* Remove text from PREV/NEXT/PAUSE buttons, to avoid block invalidation after language change.

* Reposition bullets element below buttons (no visible change).

* Remove localization of text for initial autoplay Pause button state. All a11y label localization will be addressed as a later block of work.

* Removing unneeded CSS now that buttons have no inner content.

* Fix WordAds block alignment

Remove content floating by removing conflicting align{left,right,center}
classes.

Gutenberg styling handles left/right alignment.

Apply center alignment based on data-align attributes.

* Jetpack Blocks: update blocks for the final 7.1 release.


Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
Co-authored-by: Enej Bajgoric <enej.bajgoric@gmail.com>
Co-authored-by: Jefferson Rabb <j.max.rabb@gmail.com>
Co-authored-by: Rocco Tripaldi <tripaldir@gmail.com>
Co-authored-by: Thomas Guillot <thomasguillot@users.noreply.github.com>
Co-authored-by: Mikael Korpela <mikael@ihminen.org>
Co-authored-by: Miguel Torres <miguelmariatorresrojas@gmail.com>
Co-authored-by: Michael Turk <gititon@users.noreply.github.com>
@simison simison mentioned this pull request Mar 5, 2019
Closed
@jeffersonrabb jeffersonrabb mentioned this pull request Mar 6, 2019
Merged
@jeherve jeherve mentioned this pull request Aug 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simison simison simison approved these changes

@sirreal sirreal sirreal left review comments

@ockham ockham Awaiting requested review from ockham

@scruffian scruffian Awaiting requested review from scruffian

@davidakennedy davidakennedy Awaiting requested review from davidakennedy

Assignees

@jeffersonrabb jeffersonrabb

Labels
Accessibility (a11y) [Pri] BLOCKER Slideshow [Type] Bug
Projects
None yet
Milestone
Jetpack: 7.1
Development

Successfully merging this pull request may close these issues.

Slideshow Block Invalidated on Save On WP.com
5 participants
@jeffersonrabb @matticbot @simison @jeherve @sirreal