Fix view more projects link on public gallery #20321
Conversation
|
I'm not sure how this test passed with the original change: 🤔 |
|
ooh, actually, that's a different "View More" button, not the link for specific project types. I'll add some coverage for the link. |
davidsbailey
left a comment
davidsbailey
left a comment
There was a problem hiding this comment.
Hi Erin,
I believe the only one of these you need to fix is the viewMore, but there is one I'm not totally sure on.
I've looked for an article explaining when to bind or not, but none offer a super concise description. my understanding though is that you only need it for a method which you pass as a prop to another react component (e.g. onClick).
@islemaster, have you used the tool described at https://reactjs.org/blog/2017/04/07/react-v15.5.0.html#migrating-from-reactcreateclass for these conversions? Have you found a good article explaining when to bind or not?
| maxNumProjects: nextProps.projectList ? nextProps.projectList.length : 0 | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
react life cycle methods do not need to be bound to this, so you can safely leave this method how it was before.
| }; | ||
|
|
||
| renderProjectCardList(projectList, max) { | ||
| renderProjectCardList = (projectList, max) => { |
There was a problem hiding this comment.
binding renderProjectCardList is also unnecessary
| * completed and the done handler has been run (if successful). | ||
| */ | ||
| fetchOlderProjects() { | ||
| fetchOlderProjects = () => { |
There was a problem hiding this comment.
I suspect this binding is unnecessary because the function that's calling it already has this bound correctly, but I'm not sure. @islemaster may know. Better safe than sorry, so fine to keep this the way you have it in this PR for now.
There was a problem hiding this comment.
That's correct. It's not really harmful to bind it here, but you don't need to bind functions that are:
- React lifecycle methods (unless you call them manually at some point)
- Only called with context, i.e.
this.methodName()
|
Nice find @Erin007 ! Since Brad is out today I think it's fine to get this merged without his answers to my questions. |
|
Merging so this fix can go in and people can see more projects, but will follow up by cleaning up functions that don't need fat arrows and adding test coverage for the view more link. |
|
I haven't used the |
3 participants
I missed a few fat arrow functions that relied on
thiswhen converting ProjectAppTypeArea in #20211, which broke the view more projects link in the public gallery.This PR fixes the broken link and updates all the functions that rely on
thisto the new syntax.