Fix important issues and start updating code base#49
Fix important issues and start updating code base#49TheLukaszNs wants to merge 3 commits intoCodeLanka:developfrom
Conversation
* Patch & Improvement Fixed a problem with non-displayed items and added some improvements * Start updating code base & update styles
|
I was already on it dude. |
|
Oh, I saw You were working on caching images, weren't you? I didn't touch this issue. |
|
I see You done much more work than me. I'll close my PR 😃 |
|
Yeah i am working on it. But still that if u would have commented on it that u were taking it, i would not have spent time on it. Since that issue was opened was me, it was my duty to close it asap. 😄 Thats why i worked on it. I hope u dont mind. 😄 |
|
Thanks @TheLukaszNs |
|
@TheLukaszNs @vibhavagarwal5 Hope you two can work jointly on this. Use https://gitter.im/CodeLanka/ez-net-app for fast discussions, I too will be there. |
|
@agentmilindu I think this is resolved. |
|
I am reopening this PR as of additional issues have been opened - I had to fix them and I had my code base already updated by me. |
* Patch & Improvement Fixed a problem with non-displayed items and added some improvements * Start updating code base & update styles * Finished code base update * Update loaderStyle.js
|
Aren't these exactly the changes i have done? |
|
Yeah it's very similar as of it couldn't be done more differently. But if you make a closer look you will notice that it's made in the other way. It also fixes #51 |
| import detailScreen from './detailScreen'; | ||
| import * as loaderStyle from './loaderStyle'; | ||
|
|
||
| export { |
There was a problem hiding this comment.
Actually u don't need to do this again.
Since in each of your individual style.js u have exported the function, this is a repetition.
There was a problem hiding this comment.
I know, I could just use ../styles/... import but I wanted the code to be available like this (for example):
import { loaderStyle } from '../assets/styles'and not like this (which you can still use 😄):
import loaderStyle from '../assets/styles/loaderStyle'It's up to you which one you want to use.
There was a problem hiding this comment.
for import { loaderStyle } from '../assets/styles' to be used, you do export loaderStyle instead of export default loaderStyle.
Plus to me, the code looks a complete duplicate except the boxItem.js styling 😄
There was a problem hiding this comment.
Oh, I know what you mean (I'll fix it now)! For the duplication - styles were already done, the point was just to move them to separate files - that's why it all looks so similar.
|
Ummm, I'm bit confused here. I see three PRs which kinda looks similar. Can you two please guide me through? |
|
@agentmilindu PR #52 is very much different from the other two. Its regarding the caching issue. |
|
@vibhavagarwal5 I've submitted this PR first (10 mins before you). Then I decided to close it, because You did nearly the same work as me, but - you are right here - you've created issue #47 (however you did not inform that you'll do it). Yesterday I reopened this issue, merged changes in mine fork and updated this PR. @agentmilindu those PRs are nearly the same because we were working on the same problem, but I've also fixed #51 and did some styles update. |
|
Is issue #51 really an issue? |
|
Let me go through the PRs and see. |
|
@nushydude hey, I need your help here to get these two PRs merged. |
3 participants
Fast summary
This PR resolves #45, resolves #47, closes #48 and finally resolves #51.
Code base update:
<ResponsiveImage />-><Image />imgtoassets/imagesUpdatedDeleted all instancesReact.PropTypeswithprop-types(this was actually done - we didn't use oldPropTypesfrom React)React.PropTypesassets(I don't know if we should move all styles from its' components to separated files and create index file to store them all?)