Skip to content
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

Better support for Arrays and Objects #1650

Closed
wants to merge 2 commits into from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Feb 1, 2021

Closes #1627

Edit: Just realised I still have to add support for Objects

@ghost
Copy link

ghost commented Feb 1, 2021

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by 🚫 dangerJS

@dblythy dblythy marked this pull request as draft February 1, 2021 02:56
@pmmlo
Copy link

pmmlo commented Feb 1, 2021

@dblythy line 260 should technically be else if not if. It'll save an operation if array is null.

Also, why is line 289 set as } else if (type === 'Boolean') {?
Shouldn't it be } else if (val && val.__type === 'Boolean') {?
Or, at least } else if (val.__type === 'Boolean') {.

Edit: Line 289 is from 80d42a8. I think line 287 in
ac8e566.

@dblythy
Copy link
Member Author

dblythy commented Feb 1, 2021

Thank you for looking over this @pmmlo! I'll make the changes

@pmmlo
Copy link

pmmlo commented Feb 1, 2021

You may want to consider this for Object type. I tried to keep it consistent with your coding style. I wrote this in comments, so I can't promise that it's error free.

Also, I think you missed a contentArray.push for date and boolean subtypes.

Edit: Ok, I ran a couple tests, and made some additional edits.
For the subtype Date condition, why do you have the new Date(val)? That may cause issues with certain UUIDs.
Also, the child condition with && val.__type should be unnecessary because you checked for the subtype in the parent condition.

...
        } else if (val && val.__type === 'Date' || new Date(val)) {
          if (typeof val === 'object' && val.__type) {
            value[i] = new Date(val.iso);
            val = value[i];
          } else if (typeof val === 'string' && new Date(val)) {
            value[i] = new Date(val);
            val = value[i];
          }
          contentArray.push(dateStringUTC(val));
          this.copyableValue += dateStringUTC(val);
        }
...

Here is my recommendation:

...
        } else if (val && val.__type === 'Date') {
          if (typeof val === 'object') {
            value[i] = new Date(val.iso);
            val = value[i];
          } else if (typeof val === 'string') {
            value[i] = new Date(val);
            val = value[i];
          }
          contentArray.push(dateStringUTC(val));
          this.copyableValue += dateStringUTC(val);
        }
...

Here is the Array and Object cell type conditions together. I tested some of the conditions, so fairly confident there are no typos.

    } else if (type === 'Array') {
      this.copyableValue = '';
      contentArray.push('[');
      for (var i=0;i<value.length;i++) {
        let val = value[i];
        if (val === null) {
          this.copyableValue += content = '(undefined)';
          classes.push(styles.empty);
        } else if (val === '') {
          contentArray.push(<span>&nbsp;</span>);
          classes.push(styles.empty);
        } else if (val && val.__type === 'Pointer') {
          const object = new Parse.Object(val.className);
          object.id = val.objectId;
          value[i] = object;
          val = object;
          contentArray.push(onPointerClick ? (
            <a href='javascript:;' onClick={onPointerClick.bind(undefined, val)}>
              <Pill value={val.id} />
            </a>
          ) : (
            val.id
          ));
          this.copyableValue += val.id;
        } else if (val && val.__type === 'Date') {
          if (typeof val === 'object') {
            value[i] = new Date(val.iso);
            val = value[i];
          } else if (typeof val === 'string') {
            value[i] = new Date(val);
            val = value[i];
          }
          contentArray.push(dateStringUTC(val));
          this.copyableValue += dateStringUTC(val);
        } else if (val && val.__type === 'File') {
          const fileName = val._url ? getFileName(val) : 'Uploading\u2026';
          contentArray.push(<Pill value={fileName} />);
          this.copyableValue += fileName;
        } else if (val && val.__type === 'Boolean') {
          contentArray.push(val)
          this.copyableValue += val;
        } else {
          contentArray.push(JSON.stringify(val));
          this.copyableValue += JSON.stringify(val);
        }
        if (i+1 != value.length) {
          contentArray.push(',');
        }
        contentArray.push(']');
      }
    } else if (type === 'Object') {
        if (value === null) {
          this.copyableValue = '(undefined)';
          classes.push(styles.empty);
        } else if (value === '') {
          content = <span>&nbsp;</span>;
          classes.push(styles.empty);
        } else if (value && value.__type === 'Pointer') {
          const object = new Parse.Object(value.className);
          object.id = value.objectId;
          value = object;
          content = onPointerClick ? (
            <a href='javascript:;' onClick={onPointerClick.bind(undefined, value)}>
              <Pill value={value.id} />
            </a>
          ) : (
            value.id
          );
          this.copyableValue += value.id;
        } else if (value && value.__type === 'Date') {
          if (typeof value === 'object') {
            value = new Date(value.iso);
          } else if (typeof value === 'string') {
            value = new Date(value);
          }
          content = dateStringUTC(value);
          this.copyableValue = dateStringUTC(value);
        } else if (value && value.__type === 'File') {
          const fileName = value._url ? getFileName(value) : 'Uploading\u2026';
          content = <Pill value={fileName} />;
          this.copyableValue = fileName;
        } else if (value && value.__type === 'Boolean') {
          content = value;
          this.copyableValue = value;
        } else {
          content = JSON.stringify(value);
          this.copyableValue = JSON.stringify(value);
        }
    } else if (type === 'Bytes') { 

@dblythy
Copy link
Member Author

dblythy commented Feb 1, 2021

@pmmlo the style is a bit messy - I was trying to be consistent with the original code. I can share my branch with you if you're interested in contributing to this PR? If not no worries - i'll add your changes in later.

@pmmlo
Copy link

pmmlo commented Feb 1, 2021

@pmmlo the style is a bit messy - I was trying to be consistent with the original code.
Makes sense. Thanks for contributing.

I can share my branch with you if you're interested in contributing to this PR? If not no worries - i'll add your changes in later.
Nah it's alright. I just wanted to respond to your github comment. Looks really good. Feel free to ping me as needed.

BTW I'm now seeing the end bracket also needs to move out of the for loop. Otherwise, there will be end brackets after each array entry.
This line: contentArray.push(']');

@dblythy
Copy link
Member Author

dblythy commented Feb 1, 2021

Ok, all good @pmmlo. I'll work on your suggestions. I appreciate your support. I'm not a react developer so I've sorta winged this feature together, as it's frustrated me not having the array support. Any suggestions from more experienced developers are always welcomed. Thanks mate 👍

@pmmlo
Copy link

pmmlo commented Feb 2, 2021

Ok, all good @pmmlo. I'll work on your suggestions. I appreciate your support. I'm not a react developer so I've sorta winged this feature together, as it's frustrated me not having the array support. Any suggestions from more experienced developers are always welcomed. Thanks mate 👍

Don't sweat it. I am personally not a fan of react. If you are just getting into js view libraries/frameworks, you may want to check out vue.js as well.

@dblythy
Copy link
Member Author

dblythy commented Feb 2, 2021

I 100% agree. I actually code primarily with vue + parse. It’s so much cleaner than react.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better Support for Arrays and Objects
2 participants