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

Clone Reports #5011

Merged
merged 6 commits into from
Dec 20, 2024
Merged

Clone Reports #5011

merged 6 commits into from
Dec 20, 2024

Conversation

eanders
Copy link
Contributor

@eanders eanders commented Dec 17, 2024

Merging this PR

  • use the squash-merge strategy for PRs targeting a release-X branch

Description

  • This exposes a New report from this universe button or link on HUD reports and the HMIS DQ Tool.
  • Collects up Delete report and New report from this universe into an actions menu if both are present
  • Centralizes action menu for reports

Type of change

  • New feature

Checklist before requesting review

  • I have performed a self-review of my code
  • I have run the code that is being changed under ideal conditions, and it doesn't fail
  • I have updated the documentation (or not applicable)
  • I have added spec tests (or not applicable)
  • I have provided testing instructions in this PR or the related issue (or not applicable)

@eanders eanders requested review from ttoomey and dtgreiner December 17, 2024 20:47
@eanders
Copy link
Contributor Author

eanders commented Dec 17, 2024

@ttoomey and @dtgreiner if this looks like a good pattern for the action menu I'll add it to docs/code_patterns_and_conventions.md and we can begin pushing it out throughout the app.

Copy link
Contributor

@ttoomey ttoomey left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

- items = [{ link_to: { path: "#{report.index_path}?#{{filters: filter}.to_query}"}, class: ['btn', 'btn-sm', 'btn-secondary', 'btn-icon-only'], icon: :copy, label: 'New report from this universe'}]
- unless report.running?
- items << { link_to: { path: report.show_path, method: :delete, data: {confirm: "Are you sure you want to delete this report?"}}, class: ['btn', 'btn-sm', 'btn-danger', 'btn-icon-only'], label: 'Delete report'}
= render 'common/action_menu_or_button', items: items
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we want this to become a pervasive pattern, it's worth it to wrap in a helper method. I think this has advantages over calling render directly:

  • the params to the view are formalized (unlike render, which has no formal params)
  • it's clear what partial is called (it's not relative to current controller)
  • easier to swap out the rendering for something else that's more performant (a view component or content tags)
  • more discoverable(?)
  • could let us split the view in to 'action_menu' and 'action_button' to keep partials single-purpose and more maintainable
module ApplicationHelper
# ...
def action_menu_or_button(items:)
  return if items.empty?
  return render('/common/action_menu', items: items) if items.many?

  render('/common/action_button', item: items.sole) 
end
Suggested change
= render 'common/action_menu_or_button', items: items
= action_menu_or_button(items: items)

@eanders eanders merged commit 87b7f62 into release-145 Dec 20, 2024
54 checks passed
@eanders eanders deleted the ea/report-clone branch December 20, 2024 20:33
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.

3 participants