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

feat: add pointer representation by a chosen column instead of objectId #1723

Merged
merged 70 commits into from
Oct 11, 2021
Merged

feat: add pointer representation by a chosen column instead of objectId #1723

merged 70 commits into from
Oct 11, 2021

Conversation

fn-faisal
Copy link
Member

@fn-faisal fn-faisal commented Jun 3, 2021

Issue 151 link

  • The resolution implemented in this PR for this issue is to give users access to an option in the menu where they can change the default key for a given class.
  • Throughout the Dashboard, anywhere that there is a pointer to this class object, the specified key's value (chosen in the edit menu option -> change pointer key) is used instead of objectId.
  • The way the cells load data has been updated to support async loading and fetching of data. It now uses component states to hold and display updates to the browser cell.

If no key is specified, the pointer value falls back to objectId.
Screenshot 2021-06-03 at 6 31 22 PM
Screenshot 2021-06-03 at 6 31 31 PM
Screenshot 2021-06-03 at 6 31 41 PM
Screenshot 2021-06-03 at 6 32 00 PM

Caveat

There's no caching mechanism for pointer so if the same pointer value ( class objectID ) is used in multiple rows, it'll query DB for each of the new row.

@fn-faisal
Copy link
Member Author

I will revert the package-lock.json but I'd need to add sweet alert dependency.

@mtrezza
Copy link
Member

mtrezza commented Jun 3, 2021

Thanks for this PR.

Can you please rename the PR title accordingly?

@fn-faisal fn-faisal changed the title https://github.com/parse-community/parse-dashboard/issues/151 Choose which column should represent a pointer (instead of objectId) #151 Jun 3, 2021
@fn-faisal
Copy link
Member Author

Thanks for this PR.

Can you please rename the PR title accordingly?

I have updated the PR title.

@mtrezza
Copy link
Member

mtrezza commented Jun 3, 2021

Thank you.

  • Does each cell trigger a separate query to fetch the representation, or is that done together with the paging mechanism as a bulk-fetch?
  • Where is the information about the pointer key stored? Is it sustainable enough?
  • Do we need the SweetAlert component? We already have existing components to achieve the same.

@fn-faisal
Copy link
Member Author

Thank you.

  • Does each cell trigger a separate query to fetch the representation, or is that done together with the paging mechanism as a bulk-fetch?
  • Where is the information about the pointer key stored? Is it sustainable enough?
  • Do we need the SweetAlert component? We already have existing components to achieve the same.
  • For each cell, if the type is pointer and the pointer key is not objectId, it fetches the object using the get method of the Parse Query object.
  • To store pointer data, localStorage is used to store Class name as key and the key as value.
  • A similar PR was made using b4a dash and it has sweet alert so I went with it and this PR is based off of that one, but I can revert this to Parse Dashboard specific alter dialogue.

@mtrezza
Copy link
Member

mtrezza commented Jun 3, 2021

For each cell, if the type is pointer and the pointer key is not objectId, it fetches the object using the get method of the Parse Query object.

Can we make this more efficient and do a single query for all objects in sync with the paging mechanism?

A similar PR was made using b4a dash and it has sweet alert so I went with it and this PR is based off of that one, but I can revert this to Parse Dashboard specific alter dialogue.

Yes, it would be good if you could use the components we have. Every dependency is a cost factor for maintenance, security, etc. so we want to avoid adding new ones if they are not necessary.

@fn-faisal
Copy link
Member Author

fn-faisal commented Jun 4, 2021

I've pushed a commit that will optimize the query. The second query has been removed and instead I've added include and select on the first query that fetches the data. I'll update the dialog in the upcoming commits.

@fn-faisal
Copy link
Member Author

Screenshot 2021-06-05 at 12 08 52 AM

The dialogue to change pointer value has been updated. The sweet alert2 deps have been removed.

@mtrezza
Copy link
Member

mtrezza commented Jun 4, 2021

Great, kindly let me know when this is ready for review.

@fn-faisal
Copy link
Member Author

I've cleaned up the code and the PR is ready to be reviewed.

package-lock.json Outdated Show resolved Hide resolved
src/components/BrowserCell/BrowserCell.react.js Outdated Show resolved Hide resolved
src/components/BrowserCell/BrowserCell.react.js Outdated Show resolved Hide resolved
src/components/BrowserCell/BrowserCell.react.js Outdated Show resolved Hide resolved
src/dashboard/Data/Browser/BrowserToolbar.react.js Outdated Show resolved Hide resolved
src/dashboard/Data/Browser/PointerKeyDialog.react.js Outdated Show resolved Hide resolved
src/dashboard/Data/Browser/PointerKeyDialog.react.js Outdated Show resolved Hide resolved
src/dashboard/Data/Browser/PointerKeyDialog.react.js Outdated Show resolved Hide resolved
src/dashboard/Data/Browser/PointerKeyDialog.react.js Outdated Show resolved Hide resolved
@fn-faisal
Copy link
Member Author

@mtrezza For the first issue regarding the pointers, I tried the steps above in the following video but could not reproduce the issue.

Screen.Recording.2021-09-29.at.9.31.49.PM.mov

@mtrezza
Copy link
Member

mtrezza commented Sep 29, 2021

Thanks, I'll take another look. Maybe you can take a look this meanwhile.

@mtrezza mtrezza changed the title feat: add pointer representation by a chosen column instead of objectId #151 feat: add pointer representation by a chosen column instead of objectId Sep 29, 2021
@mtrezza
Copy link
Member

mtrezza commented Oct 6, 2021

To make sure it doesn't get overlooked, the issue review is on hold because of #1723 (comment)

@fn-faisal fn-faisal requested a review from mtrezza October 7, 2021 10:26
@fn-faisal
Copy link
Member Author

@mtrezza I've reverted the changes for comment #1723 (comment)

@mtrezza
Copy link
Member

mtrezza commented Oct 7, 2021

Great, did you look through the whole PR if there are any other modifications that are not part of this PR?

@fn-faisal
Copy link
Member Author

@mtrezza I went through the PR and it only seems to have changes related to pointer key issue.

@mtrezza mtrezza changed the title feat: add pointer representation by a chosen column instead of objectId feat: add pointer representation by a chosen column instead of objectId Oct 7, 2021
@mtrezza mtrezza changed the title feat: add pointer representation by a chosen column instead of objectId feat: add pointer representation by a chosen column instead of objectId Oct 7, 2021
@fn-faisal fn-faisal requested a review from mtrezza October 11, 2021 14:01
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Amazing feature and great work 👏

@mtrezza
Copy link
Member

mtrezza commented Oct 11, 2021

🎉 This pull request has been released in version 3.3.0-alpha.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released-alpha Released as alpha version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants