search

rubyforgood / pet-rescue

Pet Rescue is an application making it easy to link adopters/fosters with pets. We work with grassroots pet rescue organizations to understand how we can make the most impact.
MIT License
57 stars 95 forks source link

760 shared views #828

Closed mononoken closed 2 months ago

mononoken commented 2 months ago

🔗 Issue

760

✍️ Description

I meant to get this reorganization done before the event but did not end up getting it done in time. Here is what I came up with.

There was a little bit of chaos in the views for sharing partials. There were three different places that had shared partials:

Solution:

Rails has a built in path into #render for shared partials, so we should use that directory. It is views/application/. https://edgeguides.rubyonrails.org/layouts_and_rendering.html#template-inheritance

I moved partials from views/shared and views/partials into views/application to follow this convention. However, there were also many partials that are not even shared that were in these directories, so I moved some to their used namespace instead. There was at least one instance of a partial that was not even getting used, so I deleted it.

Next, views/components feels like it should be for ViewComponents but we are not using ViewComponents directly. We are using nice_partials which does use ViewComponents under the hood. I moved all partials not using nice_partials out of views/components.

I think we could consider renaming views/components to views/nice_partials to make this more explicit and less confusing.

I think we should consider adopting ViewComponents at some point as well and dropping nice_partials but that is a separate issue.

cflipse commented 2 months ago

Rails has a built in path into #render for shared partials, so we should use that directory. It is views/application/. https://edgeguides.rubyonrails.org/layouts_and_rendering.html#template-inheritance

Careful with this one. Using an unscoped rendering name is most useful if you think that you're going to want to override a partial within a particular view scope. Using the "empty_list" example from the docs you linked, this allows for cases where you might choose to have an additional pets/_empty_list.html.erb and your views rendered under pets would magically use that partial instead.

Otherwise, unscoped partials mostly manage to ... not communicate where they live.

I moved partials from views/shared and views/partials into views/application to follow this convention. However, there were also many partials that are not even shared that were in these directories, so I moved some to their used namespace instead. There was at least one instance of a partial that was not even getting used, so I deleted it.

:100:

Next, views/components feels like it should be for ViewComponents but we are not using ViewComponents directly. We are using nice_partials which does use ViewComponents under the hood. I moved all partials not using nice_partials out of views/components.

Unless it's gem dependencies lie, it's fully it's own thing. It's possibly using the same render api that VC uses, but there doesn't appear to be a common base otherwise.

I think we could consider renaming views/components to views/nice_partials to make this more explicit and less confusing.

even views/partials might be fine -- nice_partials is more than a bit wordy to get out.

I think we should consider adopting ViewComponents at some point as well and dropping nice_partials but that is a separate issue.

I'd be in favor of that. VC can do some really nice things around encapsulating view specific logic in a way that actually lets you test things in the view without iterating heavyweight browser tests.

mononoken commented 2 months ago

Rails has a built in path into #render for shared partials, so we should use that directory. It is views/application/. https://edgeguides.rubyonrails.org/layouts_and_rendering.html#template-inheritance

Careful with this one. Using an unscoped rendering name is most useful if you think that you're going to want to override a partial within a particular view scope. Using the "empty_list" example from the docs you linked, this allows for cases where you might choose to have an additional pets/_empty_list.html.erb and your views rendered under pets would magically use that partial instead.

Otherwise, unscoped partials mostly manage to ... not communicate where they live.

The overwriting seems like it would be desired in that example though, right? I imagined a similar scenario where we would have a shared views/application/_card.html.erb partial and if a resource wanted a customized card then they would have views/pets/_card.html.erb to overwrite the default shared one.

I do agree that it does obscure the location, but I also think that since it is Rails behavior and it's the only "shared" path included with render, that would help unobscure it.

Do you have a preference for this shared directory, @cflipse ? I have seen a decent amount of views/shared/ in Rails projects, but I found views/application/ from a thoughtbot project and liked the brevity.

Unless it's gem dependencies lie, it's fully it's own thing. It's possibly using the same render api that VC uses, but there doesn't appear to be a common base otherwise.

Thank you for catching that. I could have sworn I saw it as a dependency. I think I was misremembering from seeing VC in the nice_partials gemfile.

I think we could consider renaming views/components to views/nice_partials to make this more explicit and less confusing.

even views/partials might be fine -- nice_partials is more than a bit wordy to get out.

I like views/partials for brevity, but I do think the explicitness of views/nice_partials could be useful. I also worry if it is just named views/partials then it will just end up with regular partials mixed in there over time too from confusion. Although maybe the separation of regular shared partials and the nice_partials ones is not necessary as I am thinking it should be?

mononoken commented 2 months ago

I've updated this one so now so it should be ready to merge: