Skip to content

Translate Widget: update layout to be responsive.#7296

Merged
dereksmart merged 4 commits intomasterfrom
update/google-translate-layout
Oct 30, 2017
Merged

Translate Widget: update layout to be responsive.#7296
dereksmart merged 4 commits intomasterfrom
update/google-translate-layout

Conversation

@jeherve
Copy link
Copy Markdown
Member

@jeherve jeherve commented Jun 2, 2017

Fixes #5962

Changes proposed in this Pull Request:

This PR includes 2 changes:

  1. Change the default layout of the widget displayed by Google. Until now the widget used the "Dropdown Only" mode. It now uses
    the "Vertical" mode. This layout is more compatible with mobile devices.
  2. Create a new filter, jetpack_google_translate_widget_layout, allowing one to switch between layouts if needed:
/**
 * Change the Google Translate Widget layout.
 */
function jeherve_dropdown_only_translate_layout() {
        return 'google.translate.TranslateElement.InlineLayout.SIMPLE';
}
add_filter( 'jetpack_google_translate_widget_layout', 'jeherve_dropdown_only_translate_layout' );

Before:

screenshot 2017-06-02 at 14 35 02

After:

screenshot 2017-06-02 at 14 45 21

Testing instructions:

  1. Make sure the widget is displayed properly on desktop and mobile devices. I'd recommend using an incognito window when switching to the branch to test , as things tend to get cached.
  2. Add the snippet above to a functionality plugin and make sure the widget layout can be changed. You can also use the google.translate.TranslateElement.InlineLayout.HORIZONTAL string for another layout.

Proposed changelog entry for your changes:

  • Google Translate Widget: update layout to be responsive.

@jeherve jeherve added [Feature] Extra Sidebar Widgets [Pri] Normal [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Jun 2, 2017
@jeherve jeherve self-assigned this Jun 2, 2017
@jeherve jeherve requested a review from artpi June 2, 2017 12:45
eliorivero
eliorivero suggested changes Jun 12, 2017
View reviewed changes
modules/widgets/google-translate.php Outdated
*
* @param string $layout layout of the Google Translate Widget.
*/
'layout' => esc_js( apply_filters( 'jetpack_google_translate_widget_layout', '' ) ),
Copy link
Copy Markdown
Contributor

@eliorivero eliorivero Jun 12, 2017

Choose a reason for hiding this comment

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

On one hand, this is incompatible with what google.translate.TranslateElement accepts for the layout property, it accepts a number, not a string.
Logging in console:
google.translate.TranslateElement.InlineLayout.VERTICAL throws 0
google.translate.TranslateElement.InlineLayout.HORIZONTAL throws 1
google.translate.TranslateElement.InlineLayout.SIMPLE throws 2
So this filter should accept a number and validate that number. Otherwise a method should be devised in JS to parse the route received as string and resolve it in the google object to get the right value.

However, this is very cumbersome for users. I'd rather have a <select> controls that user can pull to choose the desired layout.

Copy link
Copy Markdown
Member Author

@jeherve jeherve Sep 21, 2017

Choose a reason for hiding this comment

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

That makes sense. I don't know that we need a UI for this though; we don't really receive requests for that option.

I updated the PR to validate the filter and only accept integers between 0 and 2. If anything else, we fallback to the default.

@eliorivero eliorivero added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jun 12, 2017
@jeherve
Translate Widget: update layout to be responsive.
9242c8a 
Fixes #5962

This commit includes 2 changes:
1. Change the default layout of the widget displayed by Google. Until now the widget used the "Dropdown Only" mode. It now uses
the "Vertical" mode. This layout is more compatible with mobile devices.
2. Create a new filter, `jetpack_google_translate_widget_layout`, allowing one to switch between layouts if needed.
@jeherve
Translate Widget: use esc_js to escape string to be used in widget
eb391ea
@jeherve
Translate Widget: only accept layout values between 0 and 2.
f54e3d0 
@see https://github.com/Automattic/jetpack/pull/7296/files#r121515867
@jeherve jeherve force-pushed the update/google-translate-layout branch from 38cf641 to f54e3d0 Compare September 21, 2017 16:33
@jeherve jeherve requested a review from a team as a code owner September 21, 2017 16:33
@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Sep 21, 2017
@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 30, 2017
@zinigor zinigor added this to the 5.5 milestone Oct 30, 2017
@dereksmart dereksmart merged commit 1a78153 into master Oct 30, 2017
@dereksmart dereksmart deleted the update/google-translate-layout branch October 30, 2017 20:54
jeherve added a commit that referenced this pull request Oct 31, 2017
@jeherve
Changelog: add #7296
f6b08ad
jeherve added a commit that referenced this pull request Mar 9, 2018
@jeherve
Translate Widget: default to responsive vertical layout.
0e93149 
Fixes #5962 again.

When I worked on #7296 to first fix this issue, I wanted to change from the old unresponsive dropdown only layout (`2`)
to the new vertical layout that would be responsive (`0`).
But while addressing other feedback on the PR I reverted that change, ant the old layout remained the default.

This fixes that. The default is 0 (vertical layout) now.
@jeherve jeherve mentioned this pull request Mar 9, 2018
Merged
dereksmart pushed a commit that referenced this pull request Mar 13, 2018
@jeherve @dereksmart
Translate Widget: default to responsive vertical layout. (#9022)
7009192 
Fixes #5962 again.

When I worked on #7296 to first fix this issue, I wanted to change from the old unresponsive dropdown only layout (`2`)
to the new vertical layout that would be responsive (`0`).
But while addressing other feedback on the PR I reverted that change, ant the old layout remained the default.

This fixes that. The default is 0 (vertical layout) now.
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artpi artpi Awaiting requested review from artpi

1 more reviewer

@eliorivero eliorivero eliorivero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jeherve jeherve

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Pri] Normal Touches WP.com Files

Projects

None yet

Milestone

5.5

Development

Successfully merging this pull request may close these issues.

Widgets: Google Translate widget popover isn't responsive

6 participants

@jeherve @eliorivero @kraftbj @zinigor @dereksmart @matticbot