Allow the user to Convert unembeddable URLs to links and try embedding again #12095
Conversation
|
Before:
After:
In this case I just put the button inside the same paragraph as the error, and added a linebreak. I think it's fine to do, and it fixes the issue. Nice work! 👍 👍 |
|
@jasmussen thank you! |
|
🙇 — and thank you! |
notnownikki
changed the title
|
Updated and added the "Try again" button to retry the embed. |
| * @param {string} buttonText The text that appears on the button to click. | ||
| */ | ||
| export async function clickButton( buttonText ) { | ||
| const button = await page.waitForXPath( `//button[contains(text(), '${ buttonText }')]` ); |
There was a problem hiding this comment.
It might be nice to have a way to scope this button somehow. Some buttons can have very general name and have multiple instances.
There was a problem hiding this comment.
Perhaps this just needs to go inline in the test? It gets tricky when you're matching the inner text of something too.
There was a problem hiding this comment.
We can iterate on it later. It should be still useful in some cases as is.
I'm only worried about that because we are planning to expose shared code for e2e tests in npm package. That means we will have to maintain all those utils :)
| // when we try to process the same request twice. | ||
| interceptionInitialized = true; | ||
| await page.setRequestInterception( true ); | ||
| page.on( 'request', async ( request ) => { |
There was a problem hiding this comment.
https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#class-page
I see page.once in this section. Could that be used instead?
There was a problem hiding this comment.
once is for responding to an event once, not only registering a handler once, unfortunately, and we want our handler to be there for all requests.
There was a problem hiding this comment.
we want our handler to be there for all requests.
All requests... within a particular file of tests? Or the entire suite?
To me, this request mocking should only take place for the duration of the tests where it's applicable. It seems we're missing a critical "tear down" step, which risks lingering test state inadvertently affecting other test files.
gziolo
added
[Block] Embed
There was a problem hiding this comment.
It works great in my testing. I tried the following:
- go offline, paste link and click try again after going online
- paste unknow Twitter link, paste GitHub link and convert from embed to link
Great work @notnownikki, thanks 🙇
| if ( mock.match( request ) ) { | ||
| await mock.onRequestMatch( request ); | ||
| return; | ||
| if ( ! interceptionInitialized ) { |
There was a problem hiding this comment.
What if two test files each want to set up their own response mocks? Wouldn't this singleton boolean global only allow the first set of mocks to "win"?
Description
On the UI that tells the user that the URL could not be embedded, this adds a button to convert the URL to a paragraph block with a link, and a button to attempt the embedding again.
How has this been tested?
Try to embed a URL that can't be embedded, press the new "Convert to link" button. Press the "Try again" button and you should see the embed proxy request again.
Screenshots
Types of changes
New feature
Checklist: