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

Edit Profile and Stimulus Modal #44

Merged
merged 8 commits into from
Apr 5, 2024
Merged

Edit Profile and Stimulus Modal #44

merged 8 commits into from
Apr 5, 2024

Conversation

Shaka-n
Copy link
Collaborator

@Shaka-n Shaka-n commented Mar 21, 2024

WIP Reason: along the way I broke the form! but it looks nice. Fixing shortly

Adds an 'edit' profile form!
Introduces ViewComponent for reusable component views!
Adds the modal component!
Fixes the logout bug!

Update: Still needs tests, but ready to go otherwise.

@Shaka-n Shaka-n marked this pull request as ready for review March 26, 2024 13:36
@Shaka-n Shaka-n changed the title [WIP] Edit Profile and Stimulus Modal Edit Profile and Stimulus Modal Mar 26, 2024
@Shaka-n Shaka-n requested a review from emmahyde April 2, 2024 18:28
class: 'fixed z-20 w-screen h-screen bg-gray-100/75',
data: { controller: 'modal',
modal_target: 'modal',
action: 'turbo:submit-end->modal#submitEnd keydown.esc@window->modal#closeWithEscape mousedown@window->modal#closeWithBackgroundClick'} do %>
Copy link
Owner

Choose a reason for hiding this comment

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

🤮

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤣 I know, it looks awful, and the connection with the actual controller actions is so tenuous.

I was thinking of writing a helper module for stuff like this to make it more readable. It would be really cool if we could make something like Phoenix's verified_routes. That way we wouldn't need to have magic strings. We could round up all the controller actions in the JS at compile time and set a global verified_stimulus_actions map. The tough part would be somehow generating all the valid event triggers to go with them... maybe we wouldn't have to though. Ending up with something like this:

$verified_stimulus_actions = { 'modal': ['closeEnd', 'submitEnd'] }

... then have a macro/resolver of some kind check if the action exists and fail more visibly if it does not:

module StimulusHelper
  def verify_action (action)
    check_for_action($verified_stimulus_actions)
  end
end

...Something like that anyway.

In the meantime, I was thinking something like this just for readability:

module StimulusHelper
  def action_list(actions)
    actions.join(' ')
  end
end

...
### modal_component.html.erb
<tag.div id:'modal-overlay;
  data: {
    controller: 'modal',
    action: StimulusHelper.action_list(["turbo:submit-end->modal#submitEnd", "keydown.esc@window->modal#closeWithEscape"])
  } do %>
  <%= content %>
  <% end %>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gonna make an issue for this actually. Don't want to hold up this PR any longer but the more I think about it the better an idea this seems. Maybe even a gem worth publishing, if I can't find one that does something similar 🤔

@@ -12,6 +12,7 @@
#
class Profile < ApplicationRecord
belongs_to :user
accepts_nested_attributes_for :user
Copy link
Owner

Choose a reason for hiding this comment

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

👌

if update_result
redirect_to profile_path(@user)
if @profile.update!(profile_params)
redirect_to profile_path(@user.unique_name), notice: 'Profile was successfully updated.'
Copy link
Owner

@emmahyde emmahyde Apr 3, 2024

Choose a reason for hiding this comment

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

During the rewiring I did to make things navigate to the unique name, I defined to_param on the User model:

def to_param
unique_name
end

And also on the Profile cuz they're basically the same thing:

def to_param
user.unique_name
end

This is what all these Rails autogenerated _path helpers call under the hood when an ActiveRecord object is passed. The default behavior takes the id off of the object, so here we customize that behavior to pluck the unique_name instead.

Long story short, I think you can just do

Suggested change
redirect_to profile_path(@user.unique_name), notice: 'Profile was successfully updated.'
redirect_to profile_path(@user), notice: 'Profile was successfully updated.'

it will call to_param if you don't specify! This is how I avoided updating every single path reference in the app 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahhhhhhhh, that makes so much sense! I was having trouble understanding that change. Ty!

@@ -13,6 +13,7 @@
</head>

<body>
<%= turbo_frame_tag 'modal'%>
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
<%= turbo_frame_tag 'modal'%>
<%= turbo_frame_tag 'modal' %>

Comment on lines +1 to +2
<%= render ModalComponent.new(title: "Editing Profile") do %>
<%= render "form", profile: @profile, user: @user %>
Copy link
Owner

Choose a reason for hiding this comment

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

omfg

@@ -14,7 +12,7 @@
<div id='friend-request-button' class='absolute -top-4 -right-2 '>
<span class='bg-blue-200 py-1 px-2 rounded-full opacity-50'>Request Friend</span>
<% if current_user == @user %>
<%= link_to 'Edit', edit_profile_path(@user), id: 'edit-profile-button', class: 'bg-blue-200 py-2 px-2 rounded-full w-9', data: { turbo_frame: 'edit-profile-modal' } %>
<%= link_to 'Edit', edit_profile_path(@user), id: 'edit-profile-button', class: 'bg-blue-200 py-2 px-2 rounded-full w-9', data: { turbo_frame: 'modal' } %>
Copy link
Owner

Choose a reason for hiding this comment

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

the previous thing I mentioned is why this navigates via the unique_name, same thing

@emmahyde
Copy link
Owner

emmahyde commented Apr 3, 2024

Honestly inspiring, I was def trying to do too many things at once. This is an awesome incremental change

@Shaka-n Shaka-n merged commit 569805f into master Apr 5, 2024
2 checks passed
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.

None yet

2 participants