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

Documentation/59549 add short documentation around our hotwire and viewcomponent usage #17250

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

jjabari-op
Copy link
Collaborator

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

Nice write up 👏 Thank you for taking the time to write it.

I left some comments about things I still do not fully understand, and some suggestions too.

For completeness, I think it would be nice to add some basic method documentation for OpTurbo::ComponentStream. Especially for the ones relying on insert_as_turbo_stream to indicate that the insert target can be changed. The best doc is the one available while writing code, even if it can go stale at some point.

Comment on lines +22 to +23
- Controllers include OpTurbo::ComponentStream module
- Provides methods for turbo-stream operations:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Controllers include OpTurbo::ComponentStream module
- Provides methods for turbo-stream operations:
- Controllers must include `OpTurbo::ComponentStream` module, which provides methods for turbo-stream operations:

- `replace_via_turbo_stream`
- `remove_via_turbo_stream`
- `update_flash_message_via_turbo_stream`
- Uses `respond_with_turbo_streams` to handle responses
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Uses `respond_with_turbo_streams` to handle responses
- Uses `respond_with_turbo_streams` to handle responses instead of `respond_to`/`render`.

Comment on lines +24 to +29
- `update_via_turbo_stream`
- `append_via_turbo_stream`
- `prepend_via_turbo_stream`
- `replace_via_turbo_stream`
- `remove_via_turbo_stream`
- `update_flash_message_via_turbo_stream`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `update_via_turbo_stream`
- `append_via_turbo_stream`
- `prepend_via_turbo_stream`
- `replace_via_turbo_stream`
- `remove_via_turbo_stream`
- `update_flash_message_via_turbo_stream`
- `update_via_turbo_stream`
- `replace_via_turbo_stream`
- `remove_via_turbo_stream`
- `modify_via_turbo_stream`
- `append_via_turbo_stream`
- `prepend_via_turbo_stream`
- `add_before_via_turbo_stream`
- `render_error_flash_message_via_turbo_stream`
- `update_flash_message_via_turbo_stream`
- `scroll_into_view_via_turbo_stream`

Reordered in the same order as method definition in app/controllers/concerns/op_turbo/component_stream.rb file, and add missing methods.

Comment on lines +51 to +59
# modifier to determine if the insert target should be modified
# relevant for append or prepend operations
def insert_target_modified?
true
end

def insert_target_modifier_id
"work-package-journals"
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't get what this "insert target" is supposed to be. It's also mentioned on line 19 "Can specify insert targets for append/prepend operations" but that does not makes things tick for me.

What happens if insert_target_modified? returns false?

%>
```

With this setup, we can creat turbo-stream updates in a rails controller quite easily:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
With this setup, we can creat turbo-stream updates in a rails controller quite easily:
With this setup, turbo-stream updates can be sent from a rails controller:

Maybe a personal taste, but I do not like subjective statements like "quite easily" in docs. It makes me feel stupid when I do not understand :)


## Usage alongside Primer Forms/Buttons

TODO: is `turbo: true` required here?
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea 😶

Copy link
Member

Choose a reason for hiding this comment

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

Should no longer be required now that turbo drive is enabled by default


# ...

def update
Copy link
Member

Choose a reason for hiding this comment

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

One thing missing in the example above is a button which triggers the update method.

Comment on lines +233 to +235
scheme: :secondary,
size: :medium,
tag: :a,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
scheme: :secondary,
size: :medium,
tag: :a,

These parameters are a distraction as they are not needed for understanding how Hotwire and Primer forms work together. Better remove them.

end
%>
```

Copy link
Member

Choose a reason for hiding this comment

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

If the below example is different from the

Suggested change
Rendering of a cancel button to remove the edition form. The button calls the `cancel_edit` action on the controller. The `cancel_edit` action sends a turbo stream `replace` to replace the journal edit form with the journal view.

It would be nice to have also the code for the cancel_edit controller action.

Comment on lines +261 to +262
const context = await window.OpenProject.getPluginContext();
this.turboRequests = context.services.turboRequests;
Copy link
Member

Choose a reason for hiding this comment

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

Sounds magic :)
Is TurboRequestsService an angular service? I guess it does not really matter as long as it works.

Copy link
Member

Choose a reason for hiding this comment

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

yes it's an Angular service

@Injectable({ providedIn: 'root' })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants