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

Updated Views::IndexGenerator to copy all partials #2452

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

goosys
Copy link
Contributor

@goosys goosys commented Nov 1, 2023

I believe this is an oversight from when the templates were split in a previous version.
Please review.

@nickcharlton
Copy link
Member

I'm not sure if we do actually want to do this — are you finding you do override stuff like pagination, or the headers? The code here hasn't changed much in years.

One other option would be to add a new generator for overriding the specifics, or we could also just do nothing and let users figure it out!

@goosys
Copy link
Contributor Author

goosys commented Dec 20, 2023

@nickcharlton
Thank you for the review.

These templates are not generated by any generator. Shouldn't we enable them to be generated by the generator as a guide for users who want to customize the templates?

One other option would be to add a new generator for overriding the specifics

I'm prepared to create a new generator and submit a pull request. In that case, what do you think would be an appropriate name for the generator?

@pablobm
Copy link
Collaborator

pablobm commented Oct 29, 2024

Initially I wasn't keen into incorporating these partials on the generator, but I'm less sure now. I think a user wanting to customise the index views will want them, so it makes sense to provide them.

@nickcharlton
Copy link
Member

Yeah, I was thinking the same, but I think it's worthwile having now.

@goosys, would you like to revisit this? I think it needs a rebase and that's probably it.

@goosys goosys force-pushed the fix-views-index-generator branch from cf188fa to 4b7b7ed Compare November 6, 2024 15:25
@goosys
Copy link
Contributor Author

goosys commented Nov 6, 2024

Thank you for checking. I rebased.

Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

Thank you!

@pablobm pablobm merged commit c1f78d0 into thoughtbot:main Nov 16, 2024
10 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.

3 participants