Add framework icon in user's list of sandboxes #453
Conversation
| <td>{moment(s.insertedAt).format('ll')}</td> | ||
| <td>{moment(s.updatedAt).format('ll')}</td> | ||
| <StatBody> | ||
| {getDefinition(s.template).Icon(35, 35) || |
There was a problem hiding this comment.
I would replace {getDefinition(s.template).Icon(35, 35) || getDefinition().Icon(35, 35)} with something like this:
At the beginning of the render body:
const Icon = getDefinition(s.template).Icon || getDefinition().Icon;In the StatBody:
<Icon width={35} height={35} />Calling a component as a function is generally not best practice, as it's not handled by React lifecycle that way.
There was a problem hiding this comment.
I also thought this (I remembered this would be better after pushed my changes) but waited to see your opinion 😃
There was a problem hiding this comment.
Oh, but this is in stateless function. So I'll replace ( with { in the loop
|
@CompuIves Any idea why sandbox list doesn't work on PRs? |
|
@lbogdan Firstly I'm not able to log in there
|
|
@radi-cho Yeah, we don't support logging in from a PR build (mostly because not https, but there are also other factors). If you really want to "login", copy the |
|
Looks good to me @radi-cho! Only thing left is the lint issue. |
|
@CompuIves Great 😃! I checked out the lint errors and I'm gonna fix them 😅 |
|
Integration tests should pass if you merge with master 😄 |
|
@CompuIves - The module path was wrong. That's why there were lint errors. Thanks to @samet-karaibryamov - he helped me a bit. The like, forks and views count are moved right. So I'm gonna fix this and the PR will be ready to merge 😄
|
|
It's ready to merge now, I hope 😃!
|
|
Perfect. Thank you very very much @radi-cho and @samet-karaibryamov!! |
What kind of change does this PR introduce?
This is feature request discussed in #449
What is the new behavior?
Added icons for every framework in users' page with sandboxes
Checklist:
@CompuIves, please test this and if there are problems share them here and I'll fix (I wasn't able to test this as well)