theodinproject
theodinproject copied to clipboard
Chore: Refactor media component
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
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
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 Life has gotten quite hectic and I'd definitely appreciate some help getting this over the line, thank you!
@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 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.
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.
I'm happy to help out if needed, apologies for letting it slide
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.
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.