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

Editor Media Google Photos: date range filter #25762

Closed
wants to merge 52 commits into from

Conversation

jakejones1984
Copy link
Contributor

@jakejones1984 jakejones1984 commented Jun 26, 2018

This is part of the wider migration from Picassa to Google Photos as per #29965.

Description

Several issues have been raised about the difficulty of locating specific photos within external media, specifically Google Photos.

This PR introduces a mechanism to allow users to filter to show photos within a given date range. This will allow for users to easily jump back in time to locate a specific folder and avoid the endless scrolling that is currently required to retrieve older photos.

Important Notes

The DateRange component from this PR is being worked on separately within a new has been deprecated in favour of the core component created in this PR #29938.

Also see #30380

Features

The date range filter will have the following features:

  • ability to select a date range (start - end)
  • add a dateFrom and dateTo field to the API query to allow WPCom to make API request for photos which fall within this range

Test Plan

Pre-testing prepation steps

You will need to be using the new Google Photos API integration to test this branch. In order to do this:

  1. Visit the Sharing page and disconnect from the existing Google Photos (Picasa)
  2. Apply the API patch in D23306-code
  3. Sandbox public-api.wordpress.com

Once done take the following steps:

  1. Visit the Sharing page (you should already be disconnected from Picassa Google Photos - if not go back and start again)
  2. Connect to Google Photos - if you followed the steps above correctly you will see a oAuth dialogue which clearly says "Google Photos" and shows the logo. If you see anything else then you did it wrong!
    screen shot 2019-01-25 at 16 58 07
  3. Visit the Media modal and select Google Photos from the media library dropdown (top left of modal)
  4. Verify that you can paginate through results ok.

You are now ready to start testing 😓...

Testing

Stop: have you followed the preparation steps (above)? If not please do that first then return here.

We are testing that you can filter your Google Photos library by a given date range using the UI provided. You will need to manually verify what are the appropriate date ranges of photos from you Google Library to test against. You will need:

  • A known date range where you do have photos
  • A known date range where you do not have photos (this can be very small, eg: 1-2 days)

Once you have this then you will need repeat the following steps anywhere where the Media Modal appears. So far we believe you will need test both of the following instances:

  1. Post editor - go to any post and click "Featured Image" to open the Media Modal
  2. Media Library - click on Media in the LHS menu in Calypso

With the Modal open, verify the following scenarios (note you can edit and copy the following checkboxes when reporting on testing results in the comments):

  • You can see photos with no dates selected
  • You can filter a known date range and see the photos you expect to see (ie: the photos that fall within the selected dates)
  • You can filter by a known date range where there are not any photos and see the "No results" message displayed
  • You can filter by a single date (ie: from only) and see dates from that date to infinity. If we have only 1 date then Google will assume the other end of the range is "infinity".
  • With a range selected you can "clear" you date filter and go back to see all photos.

Repeat the above steps on:

  • Firefox/Chrome (whichever is not your usual browser)
  • MS Edge
  • Mobile device (making your brower window smaller does not count, not does using Devtools device emulation mode!)
  • Tablet device (making your brower window smaller does not count, not does using Devtools device emulation mode!)

Other Items to Test

  • Resolution of eslint warnings (see b081382)
  • Fix of string-based ref usage in DatePicker 41ae862

Todo

  • Remove usages such as transparentize( $gray-lighten-20, 0.5 ).
  • Rebase and ensure all styles are migrated to Webpack ES Imports ( see p4TIVU-98Q-p2)
  • Display appropriate UI when no photos are found which match a Date filter. Current says You don't have any photos in your Google library which isn't true.
  • Determine most appropriate format to pass Dates (Date, Moment or ISO string) to onQueryFilterChange handler
  • Guard against only start or only end date scenarios. Should cope gracefully and fall back to "infinity".
  • Remove legacy DateRange styles from Media Library styles.scss
  • Remove legacy MediaDateRange component
  • Add ability to full clear the date filters and go back to "All dates" - needs guard against InvalidDate-InvalidDate scenario (being worked on in DateRange component - adds clear button and improves a11y #30380)
  • review merge checklist docs
  • UX on smaller viewport sizes
  • refactor MediaDateRange snapshot test to use more explicit assertions about shape
  • pull in existing master of package.json and npm-shrinkwrap to resolve conflict
  • add input fields within popover to afford manual entry for those who cannot (or prefer not to) utilise the datepicker interface

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards
  • My code has proper inline documentation.

@jakejones1984 jakejones1984 added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] In Progress [Feature] Media The media screen in Calypso, general media management, or integration with third party media. labels Jun 26, 2018
@matticbot
Copy link
Contributor

@jakejones1984 jakejones1984 changed the title Editor Media Google Photos: album / folder dropdown selector Editor Media Google Photos: date range filter Jun 26, 2018
@jakejones1984
Copy link
Contributor Author

jakejones1984 commented Jun 30, 2018

Screencaptures

Larger Screens

media_date_filtering

Smaller Screens

media_date_filtering_mobile

Update

@johngodley Two addition properties are added to the query to provide a "range" of dates to search for photos between.

  • dateFrom
  • dateTo

These are both Unix timestamps converted from from MomentJS objects using the unix() method. This should make it easier on the WPCom side. Obviously the date filtering is actually working right now and cannot do so until we upgrade to the Google Photos API. This is why I've pulled this work into a separate branch so it can be integrated later once you've switched over from Picasa. Nonetheless, I'd value your input on whether this format (ie: from and to) is what you are looking for.

@drw158 You can see the current UI and UX in the GIF above. You'll notice that I've added form text inputs as this allows users who cannot or prefer not to use the date picker to still be able to enter dates. I'd value your input on improvements, especially in regard to adding a mechanism to allow users to completely clear all date filters that are currently applied and go back to "everything".

@jakejones1984 jakejones1984 added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 1, 2018
@jakejones1984
Copy link
Contributor Author

Can anyone suggest why the CircleCI build keeps failing on the snapshot test?

Also, what's the best approach if tests from other modules (which I haven't touched) are causing the tests to fail?

@jakejones1984 jakejones1984 self-assigned this Jul 2, 2018
@johngodley
Copy link
Member

whether this format (ie: from and to) is what you are looking for

Yep, this seems like a good approach and should map nicely to params passed into the API endpoint

@johngodley
Copy link
Member

what's the best approach if tests from other modules (which I haven't touched) are causing the tests to fail

Which other tests are failing? I may be missing them, but I only see the date picker tests showing errors in CircleCI

@jakejones1984
Copy link
Contributor Author

Which other tests are failing? I may be missing them, but I only see the date picker tests showing errors in CircleCI

That is very odd. The results were different last time I looked at CircelCI results. I will fix these then notify you again.

@alisterscott
Copy link
Contributor

This branch needs a rebase. Are we continuing with this PR?

@jakejones1984
Copy link
Contributor Author

@alisterscott I'll get this rebased asap.

Are we continuing with this PR?

At the current time this cannot be merged (if I need to put a label on to indicate this please let me know) because the functionality would require us to move from Picassa to the new Google Photos API. @johngodley is keen to move to Google Photos API so when this happens then this PR will come in very useful to allow data range filtering functionality.

Please could you advise on how you'd prefer we handle this situation? Much appreciated.

@alisterscott alisterscott added [Status] Blocked / Hold and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 30, 2018
@jakejones1984
Copy link
Contributor Author

@alisterscott Rebase complete.

@getdave
Copy link
Contributor

getdave commented Jan 2, 2019

We are still intending to land this PR but we're waiting on the changes described at p4hfux-4s0-p2#comment-6708 in order to proceed.

Update to handle edge cases around selected date ranges. Now handles scenarios where dates in the range aren’t selected and replaces with the “wildcard” selection.

Any portion of the range passed as `0000-00-00` means any date.
https://developers.google.com/photos/library/reference/rest/v1/mediaItems/search#Date
This has been replaced with the core `DateRange` component
Previously the DateRange button was large on small screens. With the limited screen real estate we need to drop down to only using icons. This commit implements that whilst also removing all of the other Media Date Range styles which are now part of the core DateRange and thus no longer needed.
These tests are now covered in the core `DateRange` component.
Previously the Trigger button and the Clear “X” button we’re spaced due to a blanket CSS rule applying RHS margin to all `.button` elements. Overides this to ensure button and clear are butting up alongside each other as per a standard button group element.
Previously strings such as “InvalidDate” were being propagated upwards with `onQueryFiltersChange`. This caused logic errors further up the component tree. Fixed by checking date validity prior to propagating.
Previously if a queryFilter (eg: date range) caused there to be no Media returned then the UI would show “No photos in your library” (or simialr).

Updates so that if there is no Media when a queryFilter is applied then this is treated as a “No results” rather than “No content” issue and the appropriate UI is displayed.
ESLint was triggering warnings that had to be resolved (annoyingly in a single commit!).

Firstly, regarding usage of setState in componentDidMount. This is often true. However React docs themselves say this is valid when doing DOM interaction such as measuring widths…etc.

> …when you need to measure a DOM node before rendering something that depends on its size or position

See https://reactjs.org/docs/react-component.html#componentdidmount

Secondly, string refs were being used. Modified to use createRef() pattern.
Previously the query filters that allow for Date Range filtering were only added to the Media Modal instance of Media Library. Manually copied these across into the “main” Media library instance as well.
React has deprecated string based refs. Converted to a `createRef()` version.
Previously conditional logic and accompanying comment we’re very clear as to the intent of the conditional. Amended for clarity.
Matches existing toolbar styles more closely.
Previously the responsive behaviour of the toolbar wasn’t consistent across viewports. Moreover the alignment and centering of content was achieved via hacks such as absolute positioning and margins.

Updates to utilise flexbox for alignment in order to improve resilience of layout across various viewports sizes.

Resolves: #25762 (comment)
Previously if you selected a single date away from the current month by a few months in the past and closed the picker and then reopened it the calendar would be focused on today’s date rather than the date you just picked.

Fixes to always focus on the start date if there is not focused date explicitly set.

Resolves #25762 (comment)
Comparing with production all buttons were suddenly larger. Fixes to ensure toolbar buttons are not suddenly taller.

Resolves #25762 (comment)
Previously tests were out of sync with changes to files. Added initial date range filter test. Also updated wording of tests to avoid using the term “media query” as this is potentially confusing due to it’s significance in responsive design.
Updating tests to cover various scenarios uncovered x2 errors:

1. Non-string dates were accepted by moment’s parsing and were incorrectly added to the filter (eg: `new Date()`)
2. If either from or to were wildcards (eg: `0000-00-00`) then the filer wouldn’t be added. However we only wanrt to avoid the filter being added if BOTH the dates are wildcards.

Fixed and added tests to cover.
…s listing

Previously translated strings were broken up into separately translated strings and then concatenated to produce a full “no results” status string for the UI. This would not have been best practice as often sentances need to be compeltely reordered in other languages.

Updates to ensure strings are translated in a single go. As part of this @johngodley and I decided to remove the Media type from the no results statement as it didn’t add much value and would have added considerably verbosity to the code.

Resolves #25762 (comment)
Resolves Circle CI Lint warnings:

1. incorrect usages of let var declaration
2. 'component-event' should be listed in the project's dependencies. Run 'npm i -S component-event' to add it.

Both fixed.
@getdave getdave force-pushed the add/google-photos-date-filter branch from 58d545d to ac19fad Compare February 1, 2019 16:43
@stale
Copy link

stale bot commented Oct 29, 2019

This issue has been marked as stale and will be closed in seven days. This happened because:

  • It has been inactive in the past 9 months.
  • It isn't a project or a milestone, and hasn’t been labeled `[Pri] Blocker`, `[Pri] High`, `[Status] Keep Open`, or `OSS Citizen`.

You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Blocked / Hold [Status] Needs Rebase [Status] Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants