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

External Media: Update Google Photo Filters #16190

Merged
merged 2 commits into from
Jun 25, 2020
Merged

External Media: Update Google Photo Filters #16190

merged 2 commits into from
Jun 25, 2020

Conversation

marekhrabe
Copy link
Contributor

@marekhrabe marekhrabe commented Jun 18, 2020 •
edited
Loading

Fixes #15880

Changes proposed in this Pull Request:

  • This addresses bugs discovered by dialing down the UI for filtering and starting with a few restricted options

Screenshot

Screenshot 2020-06-18 at 18 15 56

New filtering options allow you to:

  • switch between All Photos (previously Recent Photos) and Albums
  • for All Photos - we show the date range select with predefined options
  • for Albums - we show breadcrumb links as you navigate in albums

In a separate PR, I will add an additional date range option that will allow you to customize it. Based on #15880 design

Testing instructions:

  • Open the Google Photos modal
  • Observe and test the new UI in the header of the modal - make sure it works as expected

Proposed changelog entry for your changes:

  • none

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello marekhrabe! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D45133-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link

jetpackbot commented Jun 18, 2020 •
edited
Loading

Warnings
⚠️

pre-commit hook was skipped for one or more commits
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16190

Generated by 🚫 dangerJS against 87a8b9b

@marekhrabe marekhrabe self-assigned this Jun 18, 2020
marekhrabe
marekhrabe commented Jun 18, 2020
View reviewed changes
extensions/shared/external-media/constants.js
value: DATE_RANGE_OLDER,
label: __( 'Older than a year', 'jetpack' ),
},
// TODO: Implement a UI for selecting month & year.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentionally left in, as the filtering logic is fully implemented. UI will follow in a different PR.

@marekhrabe marekhrabe marked this pull request as ready for review June 18, 2020 16:27
@marekhrabe marekhrabe mentioned this pull request Jun 18, 2020
Closed
@jancavan
Copy link

Thanks @marekhrabe! A few changes I'd like to suggest:

  • I think we should just call it Photos since we use Albums for the second option vs All albums
  • Instead of Older than a year, I think we should instead use Last 12 months to be consistent with rest of the options that say Last - x. Then if they want anything older than that, Any time can handle it. Thoughts? @lessbloat @lcollette @obenland
  • I can go with either Filter by time period or Sort by time period for the filtering options label. I think the latter is better - shorter and easier to understand (This was reviewed by Editorial. See: Slack-C029XD8PN-p1592510812364600)

@marekhrabe
Copy link
Contributor Author

All feedback addressed!

Screenshot 2020-06-22 at 13 28 01

@marekhrabe marekhrabe requested a review from a team June 22, 2020 11:29
@jancavan
Copy link

@marekhrabe Can we please use Gutenberg text fields where the label is above the field?

Examples:

Screen Shot 2020-06-22 at 3 30 25 PM

Screen Shot 2020-06-22 at 3 30 35 PM

This way, we don't take up too much horizontal space and can accommodate this design.

@jancavan
Copy link

I also noticed the "View" toggle moves a pixel up/down when switching views. See: https://cloudup.com/cb-bZ6ZVIwL

@marekhrabe
Copy link
Contributor Author

Absolutely, I will look into the styling as a followup PR soon. I plan no more changes in this one and I think we are ready for a final code review.

@marekhrabe
Update Google Photo Filters
166ccf2 
implement simpler date filter and hide other filters

update the label for recent photos

colocate filters and recent/album switch

comment out custom range

simplified date range

update recent label

change 'older' filter to 'last 12 months'

[not verified] update date range select label

remove extraneous import
@marekhrabe marekhrabe requested a review from a team as a June 23, 2020 15:02
@marekhrabe marekhrabe changed the base branch from feature/external-media to master June 23, 2020 15:03
@marekhrabe marekhrabe added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review labels Jun 23, 2020
@obenland
Copy link
Member

obenland commented Jun 23, 2020 •
edited
Loading

@jancavan How strongly do you feel about Sort by time period vs Filter by…? It was the one thing that threw me off a little when testing it just now, since we're not actually sorting the pictures. Filter feels more appropriate to me 😬

@jancavan
Copy link

jancavan commented Jun 23, 2020 •
edited
Loading

@obenland @marekhrabe I don't have strong opinions as long as we say, "... by time period". Please feel free to change it :) Thanks.

@obenland
Update filter label
87a8b9b 
Also removes unneeded import.

Props @jancavan.
@jancavan
Copy link

@obenland I think we should add events for this (if we don't have them already), to see if users ever use this? Would be nice to know which options in each dropdown they interact with. Can be in a separate PR of course.

obenland
obenland approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

Good to go from my end. Let's get it in and continue iterating!

@obenland obenland added [Extension] External Media Extend all block editor media tools to support external providers and removed [Status] In Progress [Status] Needs Team Review labels Jun 23, 2020
@obenland obenland requested a review from a team June 23, 2020 18:08
@marekhrabe
Copy link
Contributor Author

Followup work started in… #16266

@obenland
Copy link
Member

@jeherve #16069 is merged and it doesn't look like this one needs rebasing. Should be ready for your review

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jun 25, 2020
jeherve
jeherve approved these changes Jun 25, 2020
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.

If I use the view filter to switch to the Album view for example, the time period filter disappears. Is that on purpose for now?

Right now I can't say that the time period filter is really useful since it doesn't change what pictures I have access to; I still have to click on Load More if I want to access more than the last 12 pictures on my account. But I guess that will all change once we add more filters.

Approving this PR for now just in case the album use case is on purpose for now as well.

@jeherve jeherve added this to the 8.7 milestone Jun 25, 2020
@obenland
Copy link
Member

If I use the view filter to switch to the Album view for example, the time period filter disappears. Is that on purpose for now?

It is, yes, we took that over like that from Tinker. It might be that Google's API doesn't support date filtering for albums?

@obenland obenland merged commit 4c76019 into master Jun 25, 2020
@obenland obenland deleted the update/google-photo-filters branch June 25, 2020 18:21
@obenland obenland removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jun 25, 2020
@obenland
Copy link
Member

r209626-wpcom

@obenland obenland mentioned this pull request Jun 25, 2020
Merged
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

@obenland obenland obenland approved these changes

Assignees

@marekhrabe marekhrabe

Labels
[Extension] External Media Extend all block editor media tools to support external providers Touches WP.com Files
Projects
None yet
Milestone
8.7
Development

Successfully merging this pull request may close these issues.

(5P) External Media: Remove existing Google Photos filters in favor of date-range dropdown
6 participants
@marekhrabe @matticbot @jetpackbot @jancavan @obenland @jeherve