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: hide journals in page graph #6120

Merged
merged 2 commits into from Jul 25, 2022

Conversation

8N9KT
Copy link
Contributor

@8N9KT 8N9KT commented Jul 20, 2022

add toggle in page graph the hide journals page
Note: only the journals which mention page are hidden, the journals which are mentioned by the page remain visible

immagine

add "Show Journals" toggle in page-graph-inner
add :right-side-bar/show-journals in en and it (dicts.cljc)
add "include-journals" parameter to db/get-pages-that-mentioned-page
add "show-journal" parameter to graph-handler/build-page-graph
Copy link
Collaborator

@cnrpman cnrpman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@8N9KT This looks great. As you're modifying a db namespace, it would be helpful to have a test to demonstrate this feature works as described. Could you add such a test to model_test.cljs? Some tips for testing:

Happy to answer any questions as they come up

src/main/frontend/components/page.cljs Outdated Show resolved Hide resolved
- add test for get-pages-that-mentioned-page with show-journal paramenter
- comment deleted
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@8N9KT Thanks for the feature and addressing feedback! 👍 🚢

@@ -82,4 +82,33 @@
;; 1 (count a-ref-blocks)
;; (set ["b" "c"]) (set alias-names))))

(deftest ^:focus get-pages-that-mentioned-page-with-show-journal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally we remove metadata after using them since it can effect someone else trying to run the same focused tests. I don't mind removing this one but would be good to remove these in the future

@logseq-cldwalker logseq-cldwalker merged commit ef22973 into logseq:master Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants