chore: Migrate the AddressComponent to using ViewComponent #2638
Conversation
Migrating the AddressComponent to use ViewComponent instead of Mountain View. Added an excluded style to .rubocop.yml because it got triggered when committing the new component. Updated the calls to AddressComponent as well.
|
Not sure what the appropriate name should be for the PR title between Happy to update the title once that's clear 🙏🏿 |
|
Looking good! there's a small thing on these too that i'm unsure about - do we like having all the new components suffixed edit: i guess another q here is how much we want to improve as we go vs just getting mountain_view deprecated - hmm
i think chore if we dont fix it up and feat if we do? |
Addresses the review comments namely: 1. Try to avoid disabling cops if we can help it. 2. As part of fixing the cop violations, we are switching to use `sanitize` instead of `html_safe`. This way we can be sure that user entered strings are known HTML safe tags and attributes. 3. Opted to use hard coded values so that we don't have to call VCR during unit tests.
|
@lenikadali i pushed this along a bit. rough process was to load up a page that has an address on it and note the error then just work throught the errors. i think the main confusion is where files should live. right now the presenter is in the root component directory and the template is in a folder - i think this is confusing and it should all live in the same place. i've deleted the mountain_view code but now the other references to the address component need changing. does this make sense? Edit: I've done a bit of searching and maybe its not possible to have the component .rb file in the sidecar directory, wahhh https://viewcomponent.org/guide/templates.html#subdirectory |
|
Have tried the inline template style - what do you think? I think this is a lot easier to work with and reduces cognitive load but comes with the cost that unpacking it again means undoing this? But would we do that for something this simple? |
There was a problem hiding this comment.
i've deleted the mountain_view code but now the other references to the address component need changing. does this make sense?
Yup 😎 I think that is the natural next step here. Most likely my mistake was to attempt to migrate while the mountain_view code was still in place so that was a good thing to do.
Edit: I've done a bit of searching and maybe its not possible to have the component .rb file in the sidecar directory, wahhh https://viewcomponent.org/guide/templates.html#subdirectory
Not sure about this. It looked like it could cause errors as reported here
If the test can succeed, then I would be open to that setup. It's your call 😉
Re-added the ERB component file so that there's a clear separation of concerns. This also allows us to keep the code easy to read; in case the template grows bigger, it will still be readable. Fixed the Rails/OutputSafety cop by moving the sanitize call to within the template instead of trying to do it with the component Also switched to using the `address` factory instead of a string value; the value the component expects should be an instance of the Address model and consequently the `all_address_lines` method can be called on it.
lenikadali
changed the title
| address: @partner.address | ||
| %> | ||
|
|
||
| <%= render AddressComponent.new(address: @partner.address) %> |
There was a problem hiding this comment.
@lenikadali i found one more address just fyi! otherwise happy to merge if you are?
There was a problem hiding this comment.
Hahaha beat me to it 😄
Yes please merge 🙌
There was a problem hiding this comment.
oh no theres an test fail now are you ok to fix the test?
There was a problem hiding this comment.
Not sure why that is. Locally it doesn't fail though. Let me see 🤞
There was a problem hiding this comment.
Realized I was mixing the integration and controller tests 😅
Can't figure out why it is failing right now. Will look at it tomorrow with a fresher head.
| @@ -131,4 +131,4 @@ Lint/SelfAssignment: | |||
| Enabled: false | |||
| Lint/RedundantCopDisableDirective: | |||
| Enabled: false | |||
|
|
|||
There was a problem hiding this comment.
cant work out how to get rid of this but i think more faff than its worth to remove from the commit now lol
Fixed the test failures by updating the `formatted_address` method to use @raw_location. This had been overlooked in its conversion to be compatible with ViewComponent usage
|
Woo good catch! |
kimadactyl
deleted the
2270-migrate-address-component-to-view-component
branch
Work towards #2270
Description
This PR migrates the
AddressComponentto useViewComponentinstead of Mountain View.Updated the calls to
AddressComponentas well to partners' and events' index pages.Motivation and Context
From the issue:
Type of change
How Has This Been Tested?
Currently the attempt at a test in
test/components/address_component_test.rbis failing.Need to see how to display the event and partner pages in a local environment.
Huly®: PC-2637