theodinproject icon indicating copy to clipboard operation
theodinproject copied to clipboard

Chore: Refactor media component

Open mgrigoriev8109 opened this issue 1 year ago • 8 comments

Because

The AboutPage component was now used for the Contributing page, and needed a refactor.

This PR

Splits it up into a parent ViewComponent and three child components that are rendered in the MediaComponent's .erb. Also adds in the ability to pass in optional CSS classes.

Issue

Closes #3379

Additional Information

Running rspec yielded two failing tests, but they're on the submission view and I'm 99% sure they were failing on the main branch for me, so should be unrelated to these changes.

Pull Request Requirements

  • [x] I have thoroughly read and understand The Odin Project Contributing Guide
  • [x] The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
  • Feature - adds new or amends existing user-facing behavior
  • Chore - changes that have no user-facing value, refactors, dependency bumps, etc
  • Fix - bug fixes
  • [x] The Because section summarizes the reason for this PR
  • [x] The This PR section has a bullet point list describing the changes in this PR
  • [x] I have verified all tests and linters pass after making these changes.
  • [x] If this PR addresses an open issue, it is linked in the Issue section
  • [ ] If applicable, this PR includes new or updated automated tests

mgrigoriev8109 avatar Jul 08 '23 02:07 mgrigoriev8109

Haven't forgotten about this @mgrigoriev8109, I've had a look and have a few notes but haven't had the chance to do a proper review yet. I'll aim to to do it in the next 3 - 5 days, feel free to ping me if I haven't gotten to it by then

ChargrilledChook avatar Jul 11 '23 23:07 ChargrilledChook

Hey @mgrigoriev8109, sorry this has flown under the radar. I appreciate it's been a few months and your availability might be different, but do you want any help getting this over the line? or would you rather close it if you no longer have the time?

KevinMulhern avatar Oct 07 '23 14:10 KevinMulhern

@KevinMulhern Life has gotten quite hectic and I'd definitely appreciate some help getting this over the line, thank you!

mgrigoriev8109 avatar Oct 10 '23 15:10 mgrigoriev8109

@mgrigoriev8109 no worries, we'll need the branch in a good state first. Can you fix the merge conflict when you get a chance please?

After thats's resolved and pushed up, you can fix the linting errors by running bundle exec erblint --lint-all -a.

KevinMulhern avatar Oct 10 '23 15:10 KevinMulhern

@KevinMulhern fixed the merge conflict and ran the linter. A bit daunted trying to re-wrap my head around this, shouldn't have dropped it when I did, but if no one gets around to looking at this in the next month I can give it another go.

mgrigoriev8109 avatar Nov 20 '23 20:11 mgrigoriev8109

Thanks @mgrigoriev8109, I can help you through the feedback Chook left if you decide to give it another go. It's unlikely anyone else will get to it.

KevinMulhern avatar Nov 20 '23 20:11 KevinMulhern

I'm happy to help out if needed, apologies for letting it slide

ChargrilledChook avatar Nov 21 '23 07:11 ChargrilledChook

No worries, and thanks to both of you. Looking at this again half a year later I'm not sure what led me to believe this level of abstraction was a good idea :no_good:

I think I understand better what's the intended refactor here, I'll give this another go next week and throw any questions I have into the #top-contributors channel if I get stuck, if that's alright.

mgrigoriev8109 avatar Nov 22 '23 20:11 mgrigoriev8109

Alright I had a second kid and now got a feeling I'll never get around to this one. Looked it over briefly and with how much I misunderstood what was being asked of the refactor I really think the simplest solution would be just to close out the PR and have someone excited about grabbing a ticket take a stab at it.

mgrigoriev8109 avatar Apr 02 '24 15:04 mgrigoriev8109