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

Implement turbo_frame_reload and turbo_frame_set_src actions #10

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

minimul
Copy link
Contributor

@minimul minimul commented Oct 29, 2022

Closes #5 and Closes #6

@marcoroth marcoroth changed the title Implement reload_turbo_frame and set_turbo_frame_src. Enhance playground with mock routes. Implement reload_turbo_frame and set_turbo_frame_src actions Oct 31, 2022
@marcoroth
Copy link
Owner

marcoroth commented Nov 1, 2022

Hey @minimul, thanks for taking this one on, I think this looks good implementation-wise!

I was thinking about the action names and I'm not quite sure if I really like them. Somehow I feel like the turbo_frame should appear first in the name. I'm not sure if I like turbo_frame_reload and turbo_frame_set_src better...

Do you have a strong opinion on that?

From a readability/reading-flow point of view I think reload_turbo_frame and set_turbo_frame_src are better, but it might be better to "prefix" the actions for what they target.

@minimul
Copy link
Contributor Author

minimul commented Nov 1, 2022

Yeah, the naming actually tripped me up a bit during implementation.

I'd somewhat strongly vote for going with prefixes.

Let me know and I'll make what changes are needed.

@marcoroth
Copy link
Owner

Alright, let's go with the prefixed names then! Thank you!

@minimul
Copy link
Contributor Author

minimul commented Nov 2, 2022

OK, changes are in.

@marcoroth marcoroth changed the title Implement reload_turbo_frame and set_turbo_frame_src actions Implement turbo_frame_reload and turbo_frame_set_src actions Nov 2, 2022
Copy link
Owner

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

Thank you @minimul!

@marcoroth marcoroth merged commit 91b8137 into marcoroth:main Nov 2, 2022
marcoroth added a commit to marcoroth/turbo_power-rails that referenced this pull request Nov 19, 2022
marcoroth added a commit to marcoroth/turbo_power-rails that referenced this pull request Nov 19, 2022
@minimul minimul deleted the turbo-frame-actions branch November 29, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement turbo_frame_set_src action Implement turbo_frame_reload action
2 participants