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

Feature/blank note explorer view #493

Merged

Conversation

joeltjames
Copy link
Contributor

@joeltjames joeltjames commented Feb 18, 2021

This PR resolves #491, but creating a new "Placeholder" view.

It also extracts a FilteredNotesProvider from the current OrphansProvider. This allows us to very easily create explorer views similar to the Orphans and Blank Notes view. The new path for doing this would just require adding the requisite settings, and then adding a FoamFeature which defines the match function.

Please let me know if I've missed anything in this code change.

J.T. James added 3 commits February 17, 2021 13:25
Creates a new "Blank Note" explorer view which displays all notes that
contain only a title. When note.source.text.trim().split('\n').length
is equal to 1, a note is considered blank. This should mean that the
note contains only a title.

The UX experience is identical to that of the Orphan view. A user can
toggle between both the flat view and a nested view grouped by
directory.
Instead of just copy and pasting the orphans view into blank notes,
I created a filtered notes provider, which behaves identically to the
old orphan/blank notes providers, but allows the caller to pass in the
"filter function" which will narrow down the list of notes in the view.

This also allows us to more easily unit test the filtering logic, and
only test the flatten / nested logic in one place. It also makes it so
that when we refactor the way one of these views works (e.g. adding the
markdown preview), we don't have to make changes to the other view.
@riccardoferretti
Copy link
Collaborator

this is interesting @joeltjames
a couple of questions to put it in context:

  • currently we use the name "placeholder" for links that have not been materialized as a note, have you considered combining this concept with what you call "blank note"? (in a way, a blank note is a placeholder)
  • the logic that defines what a blank note is feels tricky, as e.g. a note with one line of content would show up as blank... maybe we could check if the note is just a heading? thinking out loud..

@joeltjames
Copy link
Contributor Author

joeltjames commented Feb 22, 2021

  • currently we use the name "placeholder" for links that have not been materialized as a note, have you considered combining this concept with what you call "blank note"? (in a way, a blank note is a placeholder)

I like the idea of combining these two, perhaps we could just merge the name, call this the "Placeholder" view, and then display placeholders in a slightly different format in the list

  • the logic that defines what a blank note is feels tricky, as e.g. a note with one line of content would show up as blank... maybe we could check if the note is just a heading? thinking out loud..
    I like the idea of checking for a title, honestly this was a quick and dirty way to get it running, and it probably makes sense to have something a little more robust.

@riccardoferretti
Copy link
Collaborator

we could just merge the name, call this the "Placeholder" view, and then display placeholders in a slightly different format in the list

I like this!

I like the idea of checking for a title

Sounds good then, let's see how it plays out. it seems a fair argument to say that a note that is only heading doesn't really have any content, and therefore is just a placeholder

@joeltjames
Copy link
Contributor Author

joeltjames commented Feb 22, 2021

@riccardoferretti I was able to get those changes made. Placeholders and Blank Notes now appear under the "Placeholder"s view. See the attached gif for a preview. The icon for placeholders is the "New File" icon, notes are the same as previous. In the "Nested Directory" view, all placeholders are grouped into a virtual directory called "Not Created".
foam-placeholders

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

This looks like a good approach. I have left a few comments, let me know your thoughts.

packages/foam-vscode/package.json Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/placeholders.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/orphans.test.ts Outdated Show resolved Hide resolved
J.T. James added 3 commits February 24, 2021 09:35
Replaced "blank notes" with "placeholders".
- Removed commented lines from packages/foam-vscode/src/features/orphans.test.ts
- Removed workspacesFsPaths and replaced with workspacesURIs
- Moved to chained array functions to determine what is a placeholder
@joeltjames
Copy link
Contributor Author

@riccardoferretti, I just pushed a few commits to address your feedback. Thanks for the suggestions.

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

There are some design considerations around the FilteredResourcesProvider class that I have been holding off to share because I wanted to clarify them in my head, but in the end I think are necessary for good integration with the codebase, let me know your thoughts - thanks!

packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/filtered-resources.ts Outdated Show resolved Hide resolved
packages/foam-vscode/src/features/placeholders.ts Outdated Show resolved Hide resolved
@joeltjames
Copy link
Contributor Author

@riccardoferretti I think I managed to clean up all of these items. Thanks again.

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

this looks good! I just left a comment about the naming used in the tests, but beyond that I think it's ready for merging

@riccardoferretti riccardoferretti merged commit f48c74c into foambubble:master Mar 2, 2021
@riccardoferretti
Copy link
Collaborator

@allcontributors add @joeltjames for code

@allcontributors
Copy link
Contributor

@riccardoferretti

I've put up a pull request to add @joeltjames! 🎉

@riccardoferretti riccardoferretti added this to the 0.11.0 milestone Mar 8, 2021
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.

New Explorer View for Blank Notes
2 participants