feat: add course end dashboard plugin slots#1658
Conversation
b200607 to
8a62a38
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1658 +/- ##
==========================================
+ Coverage 90.20% 90.23% +0.02%
==========================================
Files 343 345 +2
Lines 5747 5774 +27
Branches 1341 1342 +1
==========================================
+ Hits 5184 5210 +26
- Misses 546 547 +1
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| CourseExitViewCoursesPluginSlot.propTypes = {}; | ||
|
|
||
| export default CourseExitViewCoursesPluginSlot; |
There was a problem hiding this comment.
This doesn't need to block this PR if you didn't know, but: whenever creating new files in this repo or any MFE repo, please:
- Use TypeScript
.tsxinstead of.jsx - Use TypeScript types instead of
propTypes- propTypes have been deprecated for 8 years!
You also don't need to use default exports anymore, but you can if you want to.
There was a problem hiding this comment.
hey @bradenmacdonald thanks for the heads up. this PR fell by the wayside after the 2u shakeup.
I'm getting some CIreact/prop-types linting failures because of missing propTypes. I'm surprised that's the case. I tried to find some other PRs where someone makes this change, and I don't see anyone using any kind of /* eslint-disable react/prop-types */. Am I missing something?
There was a problem hiding this comment.
#1632 is a good example. The check is green and there are no ignore directives. I'm not sure what I'm missing.
There was a problem hiding this comment.
I haven't checked in detail, but it's likely that the linter is still requiring propTypes in .jsx files, but not in .tsx files. Could that be it? Because it's better to have propTypes than nothing, and you can't replace propTypes with types in a jsx file (unless you use JSDoc syntax but then you might as well convert to .tsx anyways)
There was a problem hiding this comment.
I changed them to .tsx files. I'm not particularly familiar with the minutiae of typescript but it seems like I needed Params in both places in order for the linter to be okay with the change
const ComponentName: React.FC<Params> = ({ p1, p2, p3}: Params) => {
Not sure why both were needed, again, the linked PR seems to get by just using the angle bracket / java-style-parameterized-class syntax, but a green checkmark is a green checkmark
There was a problem hiding this comment.
Yeah you should only need it in one or the other:
const ComponentName = ({ p1, p2, p3}: Params) => {or
const ComponentName: React.FC<Params> = ({ p1, p2, p3}) => {There was a problem hiding this comment.
But I guess there's no harm in specifying both.
c74ab08 to
11e68a0
Compare
9d0ac4f to
a936a87
Compare
* feat: add additional course end plugin slots * fix: bring plugin slot names in line with new naming scheme * refactor: change plugin files to tsx,remove propTypes * fixup! refactor: change plugin files to tsx,remove propTypes * fixup! fixup! refactor: change plugin files to tsx,remove propTypes * fixup! fixup! fixup! refactor: change plugin files to tsx,remove propTypes * fix: accidentally committed test code * fix: plugin-slot fixes * chore: add ENTERPRISE_LEARNER_PORTAL_URL env var
Add plugin slots to allow control of course end dashboard links