WIP: chore: Migrate AdminIndexComponent to use ViewComponent #2666
Conversation
Migrated the AdminIndexComponent to use ViewComponent instead of MountainView.
|
The pipeline is failing due to the renaming of the This PR is different from the first migration PR where the parts involved were simpler (not having much logic involved) |
|
oh i forgot to say! does this comment help us at all with the desire to have everything in its own folder? i did think we were otherwise going to do away with the folders completely and have everything flat in the components folder - but maybe this changes things? |
Thanks for catching that 🙌 Intended to mention it in my earlier comment but forgot 😅 So I agree about flat hiearachy but when I auto-generated the component, the directory was created as well. Just looked at the command history I have and I found this line: so it seems that in the process of figuring out how to migrate the first component, it seems I (unintentionally) created the flat hierarchy for the |
|
hopefully the autogenerator is tweakable to default to whichever version we go with! |
|
So took a look at the comment and it seems to me that we would need to rename the components that are auto-generated in order to do what the comment is saying. For now, I would lean towards the defaults, simply because for the project long-term, we wouldn't want to fight Zeitwerk and the other issues that come from having a more custom approach. So far, PlaceCal has been simple to maintain (and upgrade) because it doesn't rely on customized stuff for the most part. |
|
personally i would rather not worry about the autogenerator and just do whats best for the project, those things are always janky. not sure if this changes the answer :) |
For me no, my answer would still stand 😉 What would change is how to implement the migration: my take is that the project would be better served by sticking to defaults. If you feel otherwise, then we can add that to the the acceptance criteria for the migration and have that apply to all the migrated components (the |
|
i think im confused what the options are again. a bit late to get into this for my brain now but if you can suggest something exact that would be great :) (i.e. maybe just update this PR to be how you reckon it should be then we can review) |
Now that we have moved over to ViewComponent, we no longer need the old directory from mountain_view
|
Currently we have failing: $ rails test test/controllers/admin/collections_controller_test.rb
Error:
Admin::CollectionsControllerTest#test_root:_can_index:
ActionView::Template::Error: missing keyword: :additional_links
app/components/admin_index_component.rb:4:in `initialize'
app/views/admin/collections/index.html.erb:3:in `new'
app/views/admin/collections/index.html.erb:3
test/controllers/admin/collections_controller_test.rb:21:in `block in <class:CollectionsControllerTest>'
test/test_helper.rb:62:in `instance_exec'
test/test_helper.rb:62:in `block (4 levels) in <class:TestCase>'
8 tests, 12 assertions, 0 failures, 1 errors, 0 skipssimilar error for Not sure why this is or how to pass the |
| end | ||
|
|
||
| def model_name | ||
| properties[:model].to_s.chop.humanize |
There was a problem hiding this comment.
this properties business is mountain_view code and needs replacing - this is how it implicitly imported everything before
There was a problem hiding this comment.
ugh also surely theres a way to do this referring directly to the type itself and not with this method chain 😓 embarrased for old me lol
| # frozen_string_literal: true | ||
|
|
||
| class AdminIndexComponent < ViewComponent::Base | ||
| def initialize(additional_links:, default: []) |
There was a problem hiding this comment.
in answer to your q i think all you need to do is pass in all the params here :)
There was a problem hiding this comment.
OH sorry i read your comment again
this is making additional_links mandatory i think? i am a bit rusty but i think additional_links: [], default: [] should fix it? think you jsut need a default value. from a search it doesn't look like this is actually in use anywhere so it might be cuttable along with the comments referring to it? i think we can purge it!
Migrates the AdminIndexComponent to use ViewComponent instead of MountainView.
Checklist:
Resolves #2646
Description
Motivation and Context
From this issue:
For now, we are leaning towards the defaults as provided by the ViewComponent simply because in the long-term, we wouldn't want to fight Zeitwerk and the other issues that come from having a more custom approach. This allows us to keep PlaceCal simple to maintain (and upgrade) because it doesn't rely on a customized ViewComponent.
Type of change
How Has This Been Tested?
Unfortunately this hasn't been tested 🙈
The logic in
app/components/admin_index_component/admin_index_component.html.erbis a bit complex.