-
Notifications
You must be signed in to change notification settings - Fork 246
feat: add course end dashboard plugin slots #1658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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:
|
src/plugin-slots/CourseExitPluginSlots/CourseExitViewCoursesPluginSlot/index.jsx
Outdated
Show resolved
Hide resolved
|
||
CourseExitViewCoursesPluginSlot.propTypes = {}; | ||
|
||
export default CourseExitViewCoursesPluginSlot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.tsx
instead 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess there's no harm in specifying both.
c74ab08
to
11e68a0
Compare
9d0ac4f
to
a936a87
Compare
Add plugin slots to allow control of course end dashboard links