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

Fixes #17 - Allow Graph rendering without tags or nodes #1

Closed
wants to merge 2 commits into from

Conversation

jhhb
Copy link
Owner

@jhhb jhhb commented Aug 10, 2021

Summary of changes

  • Added a function to sanitize server response
  • updated types to reflect that the server response can contain nulls
  • Renamed OrgRoamGraphReponse to OrgRoamGraphResponse and removed an unused import in the process of renaming

QA

  • Here is a before/after video showing the result running on main versus this PR branch
before-after-17.mp4

Caveats

  • I regenerated the Next.js build output, but wasn't sure if these get regenerated as part of CI. I can easily remove this commit.

Copy link

@kirillrogovoy kirillrogovoy left a comment

Choose a reason for hiding this comment

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

Thanks, James!

@jhhb just pointing out that you've made a PR from one branch of your repo to the other branch (main) of your repo.

Merging it won't get those changes to https://github.com/org-roam/org-roam-ui

Comment on lines +2 to +4
nodes: OrgRoamNode[] | null
links: OrgRoamLink[] | null
tags: string[] | null

Choose a reason for hiding this comment

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

Are there cases when any of these properties are null/undefined?

@ThomasFKJorna was it a bug or some expected behavior?

I'd rather have those fields always present and assume they are fine on the client.

@jhhb
Copy link
Owner Author

jhhb commented Aug 11, 2021

Since there might be more discussion here (and I've made this PR against my fork 😬 ) I'm going to close this PR and just make a new issue in org-roam-ui directly. I will also follow-up on the comments I can answer there.

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.

2 participants