TheOdinProject / theodinproject

Main Website for The Odin Project
http://www.theodinproject.com
MIT License
3.78k stars 2.08k forks source link

Chore: Refactor BeliefComponent #3379

Open ChargrilledChook opened 2 years ago

ChargrilledChook commented 2 years ago

Context: As part of the contributing page Tailwind update, we re-used the AboutPage::BeliefComponent because the cards there shared a very similar structure. However, the specific naming convention no longer makes sense.

Task The component should be refactored to have a more general name, and be slightly more flexible. Currently it is only a single component that renders several children; this functionality should possibly be split into two separate components. The ability to pass in CSS classes as options would also be a plus.

Notes Per Kevin, MediaComponent may be a good abstraction here. See these links on media objects: https://tailwindui.com/components/application-ui/layout/media-objects https://getbootstrap.com/docs/4.0/layout/media-object/

mgrigoriev8109 commented 1 year ago

@ChargrilledChook can I work on this?

ChargrilledChook commented 1 year ago

@mgrigoriev8109 All yours, this was originally a reminder for myself so I'll flesh out the requirements a little but feel free to start working on it in the meantime 🤠

mgrigoriev8109 commented 1 year ago

Rad, thanks!

mgrigoriev8109 commented 1 year ago

Sweet, so far I've only renamed it to Media::MediaComponent, but will be separating it into AboutPage::MediaComponent and ContributingPage::MediaComponent.

Though I'm not 100% sure how the two will differ, I'll look at the other components in those folders and see what I come up with, and will also see if I can find CSS classes being optionally passed in through the codebase.

ChargrilledChook commented 1 year ago

@mgrigoriev8109 Noice :+1:

but will be separating it into AboutPage::MediaComponent and ContributingPage::MediaComponent.

Personally I think it would be better to remove the specificity as part of the refactor. Just MediaCardComponent is probably good enough. Part of the goal is to make the component more general and re-usable in different contexts, so adding two different versions would probably somewhat defeat the point. May be useful as an intermediate step, though.

Feel free to hit me up if you want to soundoard anything, either here or in the Contributors channel

mgrigoriev8109 commented 1 year ago

Hm, alright, so I think I tried out an approach over on this branch. I split the MediaCardComponent into three slots, with three child components. One for the image, one for the title, and one for the description - so that each could have optional css classes passed in.

The page loads without any direct errors, but the content that should be in @media doesn't seem to be getting passed from the parent MediaCardComponent to the the child components.

Tried doing a slot with a lambda, but that still didn't seem to do the trick either :/ .

  renders_one :image, -> do
   Media::ImageComponent.new(media: @media)
  end

Going to try some other ways of debugging this, since haven't done much rails / ruby debugging in a long while, but if you have any suggestions in the meantime I'd appreciate them - especially if this refactoring is off from what you were suggesting.

ChargrilledChook commented 1 year ago

@mgrigoriev8109 You shouldn't need to add slots to get the initial refactor to work; not opposed to that approach but you'll need to figure out how to make that work with the collection as well.

After a short look / tinker I was able to get the initial version to work (ie without slots) by adjusting the name spacing of the preview. The naming of modules / classes needs to match the file structure. The preview was still name-spaced under AboutPage. Let me know if that doesn't makes sense.

As to debugging - my first point of call is to look at the component on LookBook ( available at /lookbook) and follow the errors and / or inspect the rendered HTML. I was able to debug the namespacing issue because the preview wasn't showing up in Lookbook at all.

Outside of that you also have rdbg available to you, so normal investigation via binding.b will work too

mgrigoriev8109 commented 1 year ago

Rad, learned a lot of new things / remembered a lot of things doing this, let me know what changes I should make to my PR :)

mgrigoriev8109 commented 7 months ago

Doubting I'll ever wrap this up, so unassigned myself.

CouchofTomato commented 7 months ago

@ChargrilledChook

How do you want to move this forward?

Tongxin-Sun commented 1 week ago

Hi @ChargrilledChook, I’d like to give this one a try—could you assign it to me?