feat: add frontend-plugin-framework LogoSlot
#526
Conversation
| @@ -60,6 +60,7 @@ | |||
| "@fortawesome/free-regular-svg-icons": "6.6.0", | |||
| "@fortawesome/free-solid-svg-icons": "6.6.0", | |||
| "@fortawesome/react-fontawesome": "^0.2.0", | |||
| "@openedx/frontend-plugin-framework": "^1.2.3", | |||
There was a problem hiding this comment.
I'm not sure if it makes more sense to have this as a direct dependency here or to make it a peer dependency. I went with direct but I'd like to hear what others think.
| const Logo = ({ src, alt, ...attributes }) => ( | ||
| <img src={src} alt={alt} {...attributes} /> | ||
| ); | ||
|
|
||
| Logo.propTypes = { | ||
| src: PropTypes.string.isRequired, | ||
| alt: PropTypes.string.isRequired, | ||
| }; |
There was a problem hiding this comment.
I combed through the codebase and couldn't find any time where we are using a logo without a link. I have some previous commits on a WIP branch that add slots and don't remove this, but I figured I might as well take the opportunity to clean up unused code.
| href, | ||
| src, | ||
| alt, | ||
| ...attributes |
There was a problem hiding this comment.
I could only find 2 attributes being added to the logo throughout the codebase. One was className="logo", which was being done everywhere the logo was being used. I figured just being able to write <Logo instead of <Logo className="logo" made more sense, so I moved that down into the component itself.
The other attribute was less straightforward. In MobileHeader only the LinkedLogo had itemType="http://schema.org/Organization". It wasn't clear to me why that would only be in the mobile header. For now I just removed it, but I'd like to revisit this and come up with a better solution.
src/Logo.jsx
Outdated
| Logo.propTypes = { | ||
| content: PropTypes.shape({ | ||
| href: PropTypes.string.isRequired, | ||
| src: PropTypes.string.isRequired, | ||
| alt: PropTypes.string.isRequired, | ||
| }) | ||
| }; |
There was a problem hiding this comment.
I did this as a workaround since React allows editing children of a prop that is an object but not props directly. I think this could be undone if openedx/frontend-plugin-framework#84 is merged.
brian-smith-tcril
force-pushed
the
logo-slot
branch
from
a1d3815
to
1993050
Compare
brian-smith-tcril
force-pushed
the
logo-slot
branch
from
1993050
to
daffed5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #526 +/- ##
==========================================
- Coverage 70.60% 70.47% -0.14%
==========================================
Files 24 25 +1
Lines 364 359 -5
Branches 96 94 -2
==========================================
- Hits 257 253 -4
+ Misses 105 104 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
No description provided.