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

WIP: chore: Migrate AdminIndexComponent to use ViewComponent #2666

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lenikadali
Copy link
Collaborator

@lenikadali lenikadali commented Jan 9, 2025

Migrates the AdminIndexComponent to use ViewComponent instead of MountainView.

Checklist:

  • I have performed a self-review of my own code,
  • I have commented my code, particularly in hard-to-understand areas,
  • I have added tests that prove my fix is effective or that my feature works,
  • Title include "WIP" if work is in progress.

Resolves #2646

Description

Motivation and Context

From this issue:

mountain_view gem has not scaled at all well and has a bunch of weird issues with integrating with JavaScript.
We therefore need to migrate to a newer solution, view_component

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Unfortunately this hasn't been tested 🙈
The logic in app/components/admin_index_component/admin_index_component.html.erb is a bit complex.

Migrated the AdminIndexComponent to use ViewComponent
instead of MountainView.
@lenikadali lenikadali self-assigned this Jan 9, 2025
@lenikadali
Copy link
Collaborator Author

The pipeline is failing due to the renaming of the app/components/admin_index/_admin_index.html.erb to app/components/admin_index_component/admin_index_component.html.erb.

This PR is different from the first migration PR where the parts involved were simpler (not having much logic involved)

@kimadactyl
Copy link
Member

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?

@lenikadali
Copy link
Collaborator Author

lenikadali commented Jan 9, 2025

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:

$ mv app/components/address_component/address_component.html.erb -t app/components/ 

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 address_component.html.erb. I will take a proper look tomorrow 🧐

@kimadactyl
Copy link
Member

kimadactyl commented Jan 10, 2025

hopefully the autogenerator is tweakable to default to whichever version we go with!

@lenikadali
Copy link
Collaborator Author

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.

@kimadactyl
Copy link
Member

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 :)

@lenikadali
Copy link
Collaborator Author

lenikadali commented Jan 10, 2025

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 AddressComponent can be modified after the overall migration is done) 😇

@kimadactyl
Copy link
Member

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)

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.

Migrate admin_index/admin_index_component.rb to use ViewComponent instead of mountain_view
2 participants