-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix #983 verify the whole array to check Parse.Objects into generic objects #984
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
Conversation
Can you post a screenshot? |
@dannywolfmx Thanks for starting this PR. There are two parts to this issue, displaying and editing. This PR handles displaying properly. There is an error with editing an array of array of objects.
It stores correctly in the database as |
Interesting, let's me check the PR to solve the issues. |
I added some nits make sure you pull it down |
@dannywolfmx Can we get this one in? For the editing you can use similar recursion. |
@dannywolfmx @masonlee Can you guys test this out? I added a fix for editing |
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.
@dplewis with the returns and the recursive call in the middle of the larger big if, its hard to follow.
if you agree, pull detect out of the if into its own func?
@@ -43,7 +43,21 @@ let BrowserCell = ({ type, value, hidden, width, current, onSelect, onEditChange | |||
} else if (type === 'Boolean') { | |||
content = value ? 'True' : 'False'; | |||
} else if (type === 'Array') { | |||
content = JSON.stringify(value.map(val => val instanceof Parse.Object ? val.toPointer() : val)) | |||
const detectObject = (value) => { |
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.
can you pull this function out so its not in the middle of this huge if/else?
|
||
return val; | ||
}); | ||
const detectObject = (value) => { |
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.
same?
Hi guys, sorry for being late. Unfortunately I had to work on other projects so I could not devote much time to review this code. I hope this weekend to be able to finish fixing the bug commented by @dplewis. It is correct @acinader, it is better to move that block of code out of the if / else so that it can be reused. I Have a question, Wouldn't it be better to allow the developers to place comments so that other users understand better than the code does?, I find it complex understanding that makes each module to be able to locate the bug. |
@dannywolfmx I went ahead and fixed the bug I mentioned. Just need another set of eyes for testing.
Can you elaborate? |
@dannywolfmx there are no comments in that file, but there are in others. definitely feel free to comment the code! |
@dplewis Ok, I'll review it this weekend |
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.
Ugh, sorry for serial picking instead of concurrent picking, but can you rename the function please.
we've already detected that it is an array. flattenArray?
And what if the values are date? boolean? etc. is that ok? or do we need another level of factoring here 1. process array 2. process values 3. populate the cell.
here we have 2 levels instead of 3....
mmm, @acinader you are right. If the user makes an array date types, it will be processed by the "JSON.stringify" and not by the dateStringUTC. |
We Could separate that great block of if/else in another function (setContent or setValue), this way would be easier to do the same thing that does "detectArray " but including all types |
This is how dates are currently handled (with the fixes) Before Edit (From Database)
After Edit
If you refresh it goes back to how it looked in before edit (note the data in the database is the same even though they look different for view / edit. |
Danger run resulted in 1 fail; to find out more, see the checks page. Generated by 🚫 dangerJS |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closing via #1223 @dannywolfmx Thanks for getting started on this! |
Recursive function to deal with nested objects and check if some children is a Parse.Object type