-
Notifications
You must be signed in to change notification settings - Fork 0
Measure Media Weight with Performance API #12
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
base: fix-editor-unexpected-error
Are you sure you want to change the base?
Measure Media Weight with Performance API #12
Conversation
roborourke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff @ajvillegas, this is basically what I'd done / started. There's a couple of nuances to the getEntries data we could talk through when you get to that but take a look and if you want to discuss approaches let me know
kadamwhite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like how this is developing, seems to be on-track @ajvillegas . Notes:
- It seems like this is set up to only measure on click, which is cool; avoids unnecessary requests. But could we alter the UI until the data has been returned? (I assume this is part of what you've been working on).
- As a follow-up, I wonder if we can integrate this into the UI more snazzily. Inject the prompt into the sidebar on any image block, for example (this would be a after-this-version-is-working idea); or tie it properly into the prepublish checks
- Once this is done-done (and not before), it'd be cool to follow up with another PR to break apart HMMediaWeightSidebar a bit
inc/namespace.php
Outdated
| * | ||
| * @return void | ||
| */ | ||
| function get_performance_entries() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, we usually use get_ in WP core for a function which returns things; since this outputs, I'd name it render_, output_ etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I updated the function name to output_...
src/index.js
Outdated
| const listener = window.addEventListener( 'message', ( event ) => { | ||
| const receivedEntries = event.data; | ||
| // eslint-disable-next-line no-console | ||
| console.log( receivedEntries ); | ||
| // You can call a useState callback here to get the data into the component. | ||
| } ); | ||
|
|
||
| return () => { | ||
| window.removeEventListener( listener ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes an error, removeEventListener needs the type specified; and addEventListener does not return anything, so this isn't doing what you'd expect. You'd want something more like this:
const listener = ( event ) => { /* ... */ };
window.addEventListener( 'message', listener );
return () => { window.removeEventListener( 'message', listener ); };that way you're unbinding the listener by reference.
| const listener = window.addEventListener( 'message', ( event ) => { | |
| const receivedEntries = event.data; | |
| // eslint-disable-next-line no-console | |
| console.log( receivedEntries ); | |
| // You can call a useState callback here to get the data into the component. | |
| } ); | |
| return () => { | |
| window.removeEventListener( listener ); | |
| }; | |
| const listener = ( event ) => { | |
| const receivedEntries = event.data; | |
| // eslint-disable-next-line no-console | |
| console.log( receivedEntries ); | |
| // You can call a useState callback here to get the data into the component. | |
| }; | |
| window.addEventListener( 'message', listener ); | |
| return () => { | |
| window.removeEventListener( listener ); | |
| }; |
☝️ Untested but should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it should work, but removeEventListener needs the event name.
window.removeEventListener( 'message', listener );
![]()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the listener as suggested.
|
@kadamwhite @roborourke I updated the PR with your suggestions and now the listener outputs a I agree that once this is ironed out, we should break up HMMediaWeightSidebar. |
This change builds off of PR #11 and introduces a way to pull asset data from the front-end using the browser's Performance API within a post preview URL iframe.
This allows for the media sizes to reflect what is being served in the front-end and can be used to calculate media weight at different screen sizes.
The draft PR is a WIP and a proof-of-concept based on @roborourke's initial research and pseudo code.