Enabled editing the title of a collection via Stimulus

[louispt1]

I struggled to make the title appear editable on hover without it becoming very ugly - now it gets underlined.

[noracato]

Hey! Nice idea with the stimulus controller. I did not check the looks of it yet, but based on the code, here are a few comments and questions.

Could you also write a spec for the update action of the controller, please?

[noracato]

Suggested change

	if @collection.update(create_collection_params)
	if @collection.update(update_collection_params)

Let's make sure a user can't accidentally update the version, or scenarios when the update form does not yet contain them. We'd need something like update_collection_params.

[noracato]

Suggested change

@collection = Collection.find(params[:id])

We should check authentication and not just do a find_by. If you add update to the hook load_and_authorize_resource, @collection will become available and will be validated already.

[noracato]

Is it always editable for everybody?

[louispt1]

Good point! After I add update to the load_and_authorize_resource, will this work/ is this the right approach?

<span contenteditable="<%= can?(:update, @collection) %>"

[noracato]

Currently the collection is in the show action, not in the update one :) But I assume this would work.

However, as a rule we try to put the least logic in the view as possible. The more "complicated" the logic is, the more we want to put it in the controller or a component helper to keep our views clean, and have a clear idea from looking at the controller what is going on. The can? method is mostly used in before hooks of the controller, so we can classify it as something out of the league of a view.

One possibility for instance could be to create a boolean @editable = can?(:update, @collection) in the show action, and reference it in the view.

[noracato]

Or, since a Collection has only one user attached: @collection.user == current_user. Whichever you prefer.

[louispt1]

Thanks! I will have a go refactoring it in this way

[noracato]

Here I come again with some comments! Sorry for the continuous reviews. I think small issues like this one are great for learning about conventions and how to properly set up controller actions. So it becomes a bit nitpicky :)

[noracato]

Suggested change

	render json: { success: true }
	else
	render json: { success: false, errors: @collection.errors.full_messages }, status: :unprocessable_entity
	render json: @collection
	else
	render json: { errors: @collection.errors.full_messages }, status: :unprocessable_entity

We don't have to communicate the success as a parameter, because it's already communicated in the response status (like you did correctly in the Stimulus controller - response.ok).
Usually we have update actions return the updated resource in some way (e.g. redirect to the updated resource in the case of HTML, returning the serialised object in case of JSON, injecting updated resource data in case of turbo stream, etc). Not only because a next action concerning the resoucre will always follow an update action, but also for making it easier to write specs for the action.

[louispt1]

No need to apologise for the reviews! It's really helpful :) I can't learn without the nitpicking so please don't hold back

[noracato]

Super nice now! Nice work!

I added a small pencil on hover to hopefully make it more clear that the title is editable.
The rest of editable things in My Scenarios work with separate forms (clicking edit somewhere and a form appears). Even though I like your approach of direct editable content a lot more, it can be hard for users to discover as it's not consequent with the rest of the UX. That's probably why you struggled on how to make it visible and clear as well.

At one point we should probably add a edit button in the right side menu that triggers/focusses the editable content. But for now I think we should just merge it, and add it as an enhancement issue.