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

Customer Home: Promote Free Photo Library #39506

Merged
merged 11 commits into from
Feb 26, 2020

Conversation

Aurorum
Copy link
Contributor

@Aurorum Aurorum commented Feb 18, 2020

Changes proposed in this Pull Request

This promotes the Free Photo Library in the Customer Home with a GIF video demonstrating how to use it and a link to the support document on it: https://en.support.wordpress.com/free-photo-library/

Screenshot 2020-02-19 at 12 28 04

Testing instructions

It should work fine in the Customer Home, although I'm unsure when this should disappear; I feel a GIF could become distracting. I think making it dismissable isn't a bad idea, but most of the conversation about that seems to be ongoing in #39147.

Fixes #39067

@Aurorum Aurorum marked this pull request as ready for review February 18, 2020 09:38
@Aurorum Aurorum requested a review from a team as a code owner February 18, 2020 09:38
@mmtr mmtr added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 18, 2020
Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

Thanks @Aurorum!

This works well on my tests but I wonder if we could avoid to download a 1.17MB GIF file every time the Customer Home is loaded. Could we maybe embed a video in a video element using preload="none" or preload="metadata" so the video is downloaded only when the user wants to play it?

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 18, 2020

Thanks @mmtr, I'm getting this error when I try to do that:

You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file.

Is it possible to upload the video somewhere so I can use a link to embed it? Here's the video I used, by the way: https://cdn.discordapp.com/attachments/448937740399017985/679361412115464222/free-photo-library-demonstration.mp4 (note: downloads instantly)

@gwwar
Copy link
Contributor

gwwar commented Feb 18, 2020

@mmtr would you like to help with adding the video assets? I'm actually not sure if this needs to be on wpcom or if the normal Calypso asset folders would work here. cc @josephscott @sgomes if y'all had any suggestions.

@lcollette
Copy link
Contributor

Just to clarify, we won't play the video unless the customer chooses to play it. Is that correct? If this is the case, then LGTM! Thanks, @Aurorum!

@arunsathiya arunsathiya added [Feature] My Home The main dashboard for managing a WordPress.com site. OSS Citizen labels Feb 19, 2020
@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 19, 2020

Currently it's a GIF that auto-plays whenever the Customer Home is loaded. My understanding is that the intention is to switch that to a video which someone can choose to play, but that would need to be uploaded somewhere so that I could embed it I think.

@sgomes
Copy link
Contributor

sgomes commented Feb 19, 2020

@Aurorum I don't think it's any different to use a video file or a gif file through an assets import. Uploading to assets and then importing it will give you the hosted URL as a string, so you can then use it in an element. /static in the root is an option too. This element could easily be a <video> tag with autoplay turned off.

The other, less ideal option is to keep the whole thing as a gif but add some smarts, like showing a static image first and only switching to the gif when the user clicks.

I agree with the sentiment that folks have expressed that we should not be loading a 1.7MB gif without user input.

@mmtr
Copy link
Member

mmtr commented Feb 19, 2020

I think @sgomes is right. @Aurorum can you try adding the video to the assets folder and see if the code below works?

import freePhotoLibraryDemonstration from 'assets/videos/customer-home/free-photo-library-demonstration.mp4';

<video src={ freePhotoLibraryDemonstration } preload="none"/>

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 19, 2020

Sadly, no success...unless I'm doing something completely wrong?

Screenshot 2020-02-19 at 09 41 17

I've added a commit to try the thumbnail approach, but I'm running into a variety of errors with the Linter that I'm not sure how to fix, so I've just disabled it for now.

Screenshot 2020-02-19 at 10 33 19

@mmtr
Copy link
Member

mmtr commented Feb 19, 2020

Oh, seems we're missing a webpack loader for video files.

I guess that adding a mp4 extension to our file loader would do the trick:

module.exports.loader = options => ( {
	test: /\.(?:gif|jpg|jpeg|png|svg)$/i,
	use: [
		{
			loader: require.resolve( 'file-loader' ),
			options: {
				name: '[name]-[hash].[ext]',
				outputPath: 'images/',
				...options,
			},
		},
	],
}, {
	test: /\.(?:mp4)$/i,
	use: [
		{
			loader: require.resolve( 'file-loader' ),
			options: {
				name: '[name]-[hash].[ext]',
				outputPath: 'videos/',
				...options,
			},
		},
	],
} );

@sgomes would you suggest going this way further? I don't expect we'll need to load many videos in Calypso, so it might be better to just upload the video somewhere else and add a proper weback loader in the future if we start adding more videos.

@Aurorum in the meantime, I uploaded the video to the WP.com CDN so you can use this URL: https://wpcom.files.wordpress.com/2020/02/free-photo-library-demonstration.mp4

@sgomes
Copy link
Contributor

sgomes commented Feb 19, 2020

@mmtr If we're not expecting a lot of .mp4 usage, then handling this one individually, as you did, is probably the best approach! 👍

Thank you both @mmtr and @Aurorum for looking into my suggestions so quickly! 🙂

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 19, 2020

Thanks @mmtr and @sgomes! I think that I've finally got the video working now. :)

Screenshot 2020-02-19 at 12 19 10

'create stunning designs.'
) }
</p>
<Button href={ localizeUrl( 'https://support.wordpress.com/free-photo-library/' ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Linking to the support docs makes sense as a first step. @dezzie what do you think of an external icon for the button or link, similar to our support card?

Screen Shot 2020-02-19 at 1 56 09 PM

@mmtr
Copy link
Member

mmtr commented Feb 20, 2020

Posting my previous question here, in case it's missed in the review thread:

@Automattic/manage should we be concerned about non-English users? Video is quite simple and I think most of the users should be able to follow it despite showing an English interface.

@danhauk
Copy link
Contributor

danhauk commented Feb 20, 2020

@mmtr I'm not concerned about non-English users. You're right that the video is simple enough. And the "Learn more" button will take them to the support doc that has more information and still screenshots if they need it.

@gwwar
Copy link
Contributor

gwwar commented Feb 20, 2020

Good point about localization, let's maybe start with EN and add locale specific assets if this appears to help folks?

@gwwar
Copy link
Contributor

gwwar commented Feb 20, 2020

Looking pretty good so far @Aurorum 💖 If this PR thread gets a little long lived, we can always land incremental work under a feature flag.

Some larger design questions for @lcollette @dezzie @danhauk

  1. The video size here in desktop mode is pretty tiny, we may want to consider displaying the video in the primary column, or showing a larger modal when we click on it.
  2. I think we're using the native browser video player. Are there any concerns around browser support? Should we maybe standardize this to VideoPress?
  3. i18n concerns for video assets. We can start testing with EN, but this is something we'll need to consider for any feature discovery/educational pieces that might use video

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 20, 2020

For the i18n concerns, I think it's worth noting that the Help Courses are in English, and that's far more complicated than the short tutorial here. That would probably also benefit from any improvements made to i18n for videos here. :)

Screenshot_20200220-221150

@mmtr
Copy link
Member

mmtr commented Feb 21, 2020

I think we're using the native browser video player. Are there any concerns around browser support?

Should be safe to use it as long as we use a MP4 video.

See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#Browser_compatibility and https://www.w3schools.com/html/html5_video.asp (HTML Video - Browser Support)

@lcollette
Copy link
Contributor

The video size here in desktop mode is pretty tiny, we may want to consider displaying the video in the primary column, or showing a larger modal when we click on it.

Agreed, @gwwar. Let's go with a modal as a first step, then we can revisit.

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 21, 2020

Just so I understand correctly, someone would click the thumbnail of the video, and then a dialogue/modal would appear with the video at a larger size which could be dismissed, correct?

@lcollette
Copy link
Contributor

Just so I understand correctly, someone would click the thumbnail of the video, and then a dialogue/modal would appear with the video at a larger size which could be dismissed, correct?

That's correct, @Aurorum. :)

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 21, 2020

I've added the dialog, but I'm back at the issue where I'm not sure how to get the Linter working, even though keyboard accessibility seems fine (so I've just disabled it again)

Screenshot 2020-02-21 at 22 39 25

@gwwar
Copy link
Contributor

gwwar commented Feb 22, 2020

Really looking good @Aurorum I think we're almost code complete here. 🎉 Thanks for sticking with it!

Some last things we might want to touch up:

  • There's a pretty aggressive focus styling when clicking to play on the video
  • Maybe begin playing on modal open? I'll defer to design here on behavior
  • Should a background click dismiss the modal?

Some screenshots for documentation:
Mobile:
Screen Shot 2020-02-21 at 4 55 42 PM

Click on Video:
Screen Shot 2020-02-21 at 4 56 33 PM

Non-Blocking but @lcollette what do we think of the overall video content here? I think I just realized that folks might not understand how to get to the Block-Editor. We could show flows in context to home here, or have another mini clip on editing sites/pages. (Just thinking in general on how to build up these educational clips). I think we might also need annotations or audio guidance in future iterations.

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 22, 2020

There's a pretty aggressive focus styling when clicking to play on the video

I removed that. :)

Maybe begin playing on modal open?

I was thinking that too. The autoplay parameter, which I'm sure used to work quite some time ago, doesn't seem to be working for me. I'm unsure if it's a browser issue?

https://developers.google.com/web/updates/2016/07/autoplay

Although that article shouldn't really apply since the video is muted.

I think I just realized that folks might not understand how to get to the Block-Editor. We could show flows in context to home here, or have another mini clip on editing sites/pages. (Just thinking in general on how to build up these educational clips). I think we might also need annotations or audio guidance in future iterations.

I think that would tie well into the solution for #39132, and I opened a PR for that in #39617. Still very much a WIP there tho :)

@mmtr
Copy link
Member

mmtr commented Feb 24, 2020

The autoplay parameter, which I'm sure used to work quite some time ago, doesn't seem to be working for me.

Did you try using a capitalized param autoPlay? I think autoplay is not valid in a React context.

@Aurorum
Copy link
Contributor Author

Aurorum commented Feb 24, 2020

That works, thanks @mmtr! Video should now autoplay. :)

Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

Awesome work here @Aurorum, as usual! Let me know what do you think about using a functional component with hooks. Other than that, this is ready to go.

@mmtr mmtr merged commit 1404ede into Automattic:master Feb 26, 2020
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 26, 2020
@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~457 bytes added 📈 [gzipped])

name  parsed_size           gzip_size
home      +1629 B  (+0.5%)     +457 B  (+0.5%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] My Home The main dashboard for managing a WordPress.com site. OSS Citizen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Free Photo Library: Highlight feature
8 participants