Skip to content

Flickr Widget: Adds option to open Flickr images in a new tab #13120

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
Merged
jeherve merged 3 commits into from
Jul 30, 2019
Merged

Flickr Widget: Adds option to open Flickr images in a new tab #13120

jeherve merged 3 commits into from
Jul 30, 2019

Conversation

@chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Jul 24, 2019

Fixes #11405

Changes proposed in this Pull Request:

  • Previously, Flickr widget images could only be opened in the same window which directs users away from the site owners' site. This PR adds an "Open images in new tab?" option to Flickr widget settings, allowing site owners to choose whether their users are directed away from their site or not.

Testing instructions:

  1. Activate Jetpack Widgets
  2. Add the Flickr (Jetpack) widget to a widget area of your choosing
  3. Along with the other settings, check the "Open images in new tab?" checkbox
  4. View the Flickr gallery in the front-end and open one of the images.
  5. Uncheck the setting to verify images can open in the same window as well

Proposed changelog entry for your changes:

  • Add option to open Flickr gallery images in new tabs

@chickenn00dle chickenn00dle requested a review from a team July 24, 2019 07:57
@chickenn00dle chickenn00dle added the [Status] Needs Review This PR is ready for review. label Jul 24, 2019
@jetpackbot
Copy link
Collaborator

jetpackbot commented Jul 24, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against 363e4fd

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Extra Sidebar Widgets [Pri] Low labels Jul 24, 2019
jeherve
jeherve requested changes Jul 24, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I would recommend setting up your IDE so it lints code as you work; that will avoid issues with the use of spaces instead of tabs in this PR, for example.

image

You can find some instructions here:
https://github.com/Automattic/jetpack/blob/master/docs/development-environment.md#use-php-codesniffer-and-eslint-to-make-sure-your-code-respects-coding-standards

Alternatively, if you run yarn build most the necessary tools should become available to you.

@jeherve jeherve added [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 Jul 24, 2019
@chickenn00dle
Copy link
Contributor Author

chickenn00dle commented Jul 24, 2019

Ah, apologies!

Looks like I got ahead of myself there 😅

Followed up and made sure there were no issues with PHP CodeSniffer.

@chickenn00dle chickenn00dle added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jul 24, 2019
jeherve
jeherve requested changes Jul 25, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well. I would only have one minor recommendation for the look of the checkbox.

modules/widgets/flickr/form.php Outdated
Copy link
Member

@jeherve jeherve Jul 25, 2019

Choose a reason for hiding this comment

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

I would place the checkbox before the label, I think it feels more natural.

Copy link
Contributor Author

@chickenn00dle chickenn00dle Jul 25, 2019

Choose a reason for hiding this comment

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

Good call. Actually reviewing some of the other checkboxes in other Jetpack widgets it looks like the checkbox and label text is nested within label tags. Made the change for this to be in-line with this pattern.

@jeherve jeherve added [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 Jul 25, 2019
@jeherve jeherve added this to the 7.6 milestone Jul 25, 2019
@chickenn00dle
Flickr Widget: Adds option to open Flickr images in a new tab
27318e3 
Addresses this issue: #11405

Previously, the Flickr widget images could only be opened via the same window which
directs userss away from the site owners' site. This PR adds an
"Open images in new tab?" option to flickr widget settings, allowing
site owners to choose whether their users are directed away from their
site or not.
@chickenn00dle
Replaces opening spaces with tabs
3328dd0 
Previous commit was not checked with phpcs and contained several lines
tabbed with spaces. This replaces those spaces with tabs.
@chickenn00dle
Places checkbox input element before label text
363e4fd 
This commit places the checkbox input element for the "Open images in
new tab?" option" before the label text and nested within label tags to
be in line with similar Jetpack widget settings.
@chickenn00dle chickenn00dle added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jul 25, 2019
jeherve
jeherve approved these changes Jul 25, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me! It should be good to merge after a second review!

@jeherve jeherve 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 Jul 25, 2019
@jeherve jeherve merged commit d7c2c26 into Automattic:master Jul 30, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 30, 2019
jeherve added a commit that referenced this pull request Jul 30, 2019
@jeherve
Changelog: add #13120
3b144f0
jeherve added a commit that referenced this pull request Jul 30, 2019
@jeherve
7.6: running changelog (#12940)
f1d6377 
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
jeherve added a commit that referenced this pull request Jul 30, 2019
@jeherve
7.6: running changelog (#12940)
37aabd8 
* Add initial changelog / testing list changes for 7.6

* Update stable tag to 7.5.3

* changelog: add #12957

* Changelog: add #12932

* Changelog: add #12867

* Changelog: add #12823

* changelog: add #12969

* changelog: add #13012

* changelog: add #12974

* Changelog: add #13059

* Changelog: add #13079

* Changelog: add #12924

* changelog: add #12954

* Changelog: add #12959

* Changelog: add #12977

* Changelog: add #12830

* Changelog: add #12926

* Changelog: add #12958

* Changelog: add #12999

* Changelog: add #13077

* Changelog: add #13083

* Changelog: add #13087

* Changelog: add #13110

* Changelog: add #13116

* Changelog: add #13117

* Changelog: add #12821

* Changelog: add #13120

* changelog: add #13139

* Changelog: add #13143

* Changelog: add #13147

* Testing list: add section about sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@chickenn00dle chickenn00dle

Labels

[Feature] Extra Sidebar Widgets [Pri] Low [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Projects

None yet

Milestone

7.6

Development

Successfully merging this pull request may close these issues.

Flickr Widget: Open image links in a new window

4 participants

@chickenn00dle @jetpackbot @jeherve @matticbot