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

Add customizable extension partials to good_job/jobs#show view #1200

Merged
merged 9 commits into from
Feb 22, 2024

Conversation

grncdr
Copy link
Contributor

@grncdr grncdr commented Dec 28, 2023

This is a minimal implementation of the idea from #1197.

I've opted not to address the issues of exposing internal classes in this first pass, but rather just document the potential for unstable APIs. I'm open to ideas, but my opinion at the moment is that this strikes the right balance between complexity/maintenance burden for GoodJob maintainers and users.

@grncdr grncdr marked this pull request as ready for review December 28, 2023 14:59
@bensheldon
Copy link
Owner

Thank you! This looks nice. Could you please add an example in to the /demo application and also add a system test for that?

This variable is a GoodJob::DiscreteExecution except in cases where
migrations have not been run in a long time. That case didn't seem
worthy of cluttering up the comment.

See: bensheldon#928
@grncdr
Copy link
Contributor Author

grncdr commented Jan 3, 2024

Now updated with an example and a spec.

@@ -21,6 +21,12 @@ def create_migration_file
migration_template 'migrations/create_good_jobs.rb.erb', File.join(db_migrate_path, "create_good_jobs.rb")
end

def copy_empty_partials
%w[custom_job_details custom_execution_details].each do |partial|
copy_file "views/_#{partial}.html.erb", "app/views/good_job/jobs/_#{partial}.html.erb"
Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd prefer not to copy these over as part of the install generator. Initially I was excited about it, but I try to step lightly inside of other people's applications and I think I'd prefer to document it in the readme, but otherwise not force it on an application.

Copy link
Owner

Choose a reason for hiding this comment

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

This also makes me think that the path should either be:

  • views/good_job/_custom_job_details.html.erb OR
  • views/good_job/jobs/_job_details.html.erb

...because I think the custom is useful if it's at a simple root path, but otherwise it should just be "override this existing vanilla-looking path"

@gap777
Copy link
Contributor

gap777 commented Feb 9, 2024

I have an immediate use for this PR. What is the outlook for merge?

@grncdr
Copy link
Contributor Author

grncdr commented Feb 12, 2024

Thanks for updating the PR @bensheldon, I'm dealing with a newborn here and won't have time to follow up for a while 😅

@bensheldon
Copy link
Owner

@grncdr no worries at all 💖 I appreciate your help moving it this far forward 🙇🏻

@bensheldon bensheldon added the enhancement New feature or request label Feb 22, 2024
@bensheldon bensheldon changed the title Add extension partials to good_job/jobs#show view Add customizable extension partials to good_job/jobs#show view Feb 22, 2024
@bensheldon bensheldon merged commit 74b2ad8 into bensheldon:main Feb 22, 2024
20 checks passed
@gap777
Copy link
Contributor

gap777 commented Feb 22, 2024

Woohoo! When do you expect to cut a new release with these changes?

@bensheldon
Copy link
Owner

@grncdr grncdr deleted the extensible-views branch May 13, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

3 participants