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

Add a "reload" method to the frame element #206

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Conversation

coorasse
Copy link
Contributor

As proposed in #202, this PR adds a .reload() method on the frame that allows to refresh its content.

Open points:

  • I am not happy with the test. It actually does not test anything interesting. I'd like the content of the frame to change upon reloading and verify that, but I have no clue on how to do that. A bit of help or an idea would be appreciated. I left it there to give an idea of what I wanted to do, but as of now, it could be removed.
  • What if the frame has no src attribute? In my implementation, a call to reload() will simply have no effect. To me this is simple and stupid, so it should be fine.

Comment on lines 39 to 41
const srcAttribute = this.src;
this.src = null;
this.src = srcAttribute;
Copy link

Choose a reason for hiding this comment

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

We've using ES6 here, so let's do:

Suggested change
const srcAttribute = this.src;
this.src = null;
this.src = srcAttribute;
const { src } = this
this.src = null
this.src = src


const frameContent = "#loading-eager turbo-frame#frame h2"
this.assert.ok(await this.hasSelector(frameContent))
// @ts-ignore
Copy link

Choose a reason for hiding this comment

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

Why do we want this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because otherwise typescript complains that reload is not an Element method.
image
shall I cast it instead?

Comment on lines 74 to 77
this.assert.ok(await this.hasSelector(frameContent))
// @ts-ignore
await this.remote.execute(() => document.querySelector("#loading-eager turbo-frame")?.reload())
this.assert.ok(await this.hasSelector(frameContent))
Copy link

Choose a reason for hiding this comment

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

Looks like we're using tabs for some of these lines, can you switch to just spaces?

await this.remote.execute(() => document.querySelector("#loading-eager turbo-frame")?.reload())
this.assert.ok(await this.hasSelector(frameContent))
}

Copy link

Choose a reason for hiding this comment

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

Our style before didn't add an extra line, so we should cut this one too.

@kaspth
Copy link

kaspth commented Mar 12, 2021

  • I am not happy with the test. It actually does not test anything interesting. I'd like the content of the frame to change upon reloading and verify that, but I have no clue on how to do that. A bit of help or an idea would be appreciated. I left it there to give an idea of what I wanted to do, but as of now, it could be removed.

Can the src URL that's under test, be a test specific URL? That way it could maintain it's own counter and just increment that and display. So the test would check for a 2 and then a 3 (run it a third time so the post-reload isn't borked).

@coorasse
Copy link
Contributor Author

I am not sure how to achieve that. The target of src are always static HTML pages in the fixture folder.

@seanpdoyle seanpdoyle added the enhancement New feature or request label Apr 1, 2021
@acetinick
Copy link

acetinick commented Jul 31, 2021

@coorasse may i suggest an additional improvement to this PR would for the reload() function to accept an optional parameter to specify the URL to set src as.

This would be useful if you don't already have one, and want to set it as a the same URL, or if for some reason the URL changes and you want it pointing somewhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

5 participants