Agree definition of done for view_component migrations and set up code base for work to begin

[kimadactyl]

User story

As a developer I want a clear definition of done for migrating to view_component so that everything is then completely standardised.

Acceptance criteria (WIP)

• A new PR template is created for components that contains the below
• Components have a _component suffix
• Old mountain_view code is deleted/migrated
• Evaluate if each component has the best name, or if there could be a terser or more semantic one
• Generators are configured to create as much of this as possible
• Stimulus is configured to load in the JS files from the subdirectories
• Scss is left as is for now and will be reviewed in the future
• Locale information is added to the global locale file and not stored with the component
• There are minimal unit tests which do not rely on VCR
• Light refactoring is encouraged if possible but priority is to get this job done and move on
• Simple components with a template should appear as below in /app/components/...

components
├── example_component.rb            # Component definition
└── example_component.html.erb      # HTML template

• More complex components with javascript and scss can be moved into a folder:

components
├── example_component.rb            # Component definition
└── example_component
    ├── example_component.html.erb  # HTML template
    ├── example_component.js        # Optional: Stimulus controller
    └── example_component.scss      # Optional: Component-namespaced scss

Implementation notes & questions

• Do we want the _component suffix on all of these? Without is neater, with is more explicit
• How do we load in all the scss properly with depends and the like?
• Do we want a locale file per component or just add to the global one?

Implementation plan

To be written by the developer

_{Huly®: PC-2646}

[kimadactyl]

@lenikadali - i thought would be a good idea to hash this out totally before we start moving stuff, what do you think?

[lenikadali]

@kimadactyl I think this is a great idea. Will follow up on Discord 😇

[kimadactyl]

Another thought: given this is a really short template maybe we should consider having it inline in the model file, and set a guideline for when to do inline and when to break out into a folder? Unless we want to be completely consistent there too?

https://viewcomponent.org/guide/templates.html#inline

[kimadactyl]

It looks like the official reccomendation is now to not have a component suffix but not to sweat it too much - so given we are taking it from the top i suggest we remove it?

https://viewcomponent.org/adrs/0002-naming-conventions-for-view-components.html

[lenikadali]

Sample codebase I looked at in my research was the RailsDevs codebase. I would stick to what view_component does out of the box and then if we run into problems that can/need to be solved with a more custom approach, customize incrementally.

[kimadactyl]

Updated following discord conversation and now locked in i think!

[kimadactyl]

Updated following final decision in Discord to have components in root folder.

[kimadactyl]

Do you wanna take on this one next @lenikadali? Hopefully after that these can go a lot faster :D

[lenikadali]

Do you wanna take on this one next @lenikadali? Hopefully after that these can go a lot faster :D

Not sure what the outcome of this ticket should be. My understanding is that between the comments in the first PR for this issue, we've more or less agreed that definition of done is:

1. Changing the components to use ViewComponent
2. Write unit tests for each migrated component
3. Update calls across the codebase to use the ViewComponent style instead of the mountain_view style rendering call
4. Fix any tests that get broken in the process
5. If all pipelines are green, merge into main.

Let me know if I've left something out 😇

[kimadactyl]

Oh just thought there could be a PR template specifically for these checkboxes that was all! Maybe overkill? So a "New or updated component" PR template

[lenikadali]

Oh just thought there could be a PR template specifically for these checkboxes that was all! Maybe overkill? So a "New or updated component" PR template

Ah... I see what you mean 🙂 We can look into how to handle having multiple PR templates and how each gets triggered depending on the issue?

It may be overkill but the migration issue has a lot of detail that is easy to overlook. Alternatively the issue can be converted into an epic and then when migrating a particular component, the developer makes both the issue and the PR.

From what I can see, hovering over the checkboxes for the different components shows the option to convert to an issue (have done it as a test here). So the next step would be to update it with all the detail concerning this particular component's migration and then it should be ready to work on. What do you think?

[kimadactyl]

Oh I assumed it'd be like issues, that we can have multiple templates (like feature/bug/tech debt we have now) but I guess it's not quite that simple for PRs, hmm!

Yes I think this is a really nice to have and it'll speed up the process a lot. I think the PR makes more sense to have this checklist than the issues now we've agreed the steps! A quick google indicates this should be possible but maybe with some faff :D

tl;dr: issue templates and PR templates are different but i think we should have a PR template if its possible 🙏

[lenikadali]

tl;dr: issue templates and PR templates are different but i think we should have a PR template if its possible 🙏

Cool. Will take a look and see if I can open an PR for this.

[lenikadali]

tl;dr: issue templates and PR templates are different but i think we should have a PR template if its possible 🙏

Cool. Will take a look and see if I can open an PR for this.

I slept on this and realized that for this type of thing, we don't need a PR template. PR templates are used when different parts of the code require different considerations of both reviewer and implementer. As the example from GitLab shows, each PR template can be mapped to a particular type of recurring chore/fix/feature that happens in a given codebase.

For the migration from mountain_view to ViewComponents, since we are not going to do it again, changing the migration issue to an epic and moving over the details of the implementation for each specific component into its own issue would be preferable. That way we would have some form of documentation for the decisions made for each component (where they differ from the others) in the issue or the PR or both (depending on what makes sense at the time)

Will use the issue for the admin/admin_component.rb as a test for the workflow proposed earlier and if we see that something has slipped through the cracks, we can continue to refine the definition of done as needed.

[kimadactyl]

ok great. so maybe better we put component guidelines up as part of #2551 then?

[lenikadali]

ok great. so maybe better we put component guidelines up as part of #2551 then?

Yes that would make sense. I'll open a draft later today that covers what we've agreed on so far and put it (if it fits 😄) in the CONTRIBUTING.md file.

[kimadactyl]

Cool! Another option could be to chuck an .md file in the components directory actually?

Like this one: https://github.com/geeksforsocialchange/PlaceCal/blob/main/app/jobs/creating_an_importer.md

[lenikadali]

Cool! Another option could be to chuck an .md file in the components directory actually?

Like this one: https://github.com/geeksforsocialchange/PlaceCal/blob/main/app/jobs/creating_an_importer.md

That should work 🙌

[lenikadali]

PR for component guidelines is here