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

Refactor(NoteList): Reshape composite note list data structure #1810

Merged
merged 1 commit into from
Jan 6, 2020

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Jan 4, 2020

When adding tag-suggestions in the note list we create a "composite list" of notes and placeholders. The placeholders designate where headers will go, where the tag suggestions list will go, and where a message will go if there are no matching notes.

There were two issues with this structure:

  • it was difficult to type because of how its structure overlapped a NoteEntity but didn't have a key field for discriminating the type. everything works just fine but the type signatures become harder to write.
  • we were storing display information in the data property and that, as a render concern, wasn't necessary

Further, we were directly mutating notes which came from filterNotes() and the Redux state. We want to avoid directly mutating values we receive from other places in order to keep data flows clear and obvious.

In this patch we are replacing the object data type for placeholders with simpler string counterparts, moving the label text into the render function, and exchanging a mutating operation for a non-mutating one.

This is part of broader work to separate modifying the search parameters from
actually filtering the notes and was originally created as part of an
exploratory PR #1807.

Testing

This affects the tag suggestions auto-completer and the note list when searching.
Smoke test by searching in various ways: with filter text, with a tag from the tag list, with tag:something in the filter text, with the trash selected, with no notes matching the search, etc…

In early testing at one point I had the notes header and tag-suggestions header backwards, for example.

@dmsnell dmsnell requested a review from a team January 4, 2020 04:25
@dmsnell dmsnell force-pushed the refactor/note-list-composite-structure branch 3 times, most recently from 73d8643 to 9313cd8 Compare January 4, 2020 04:58
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

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

Much cleaner!

When adding tag-suggestions in the note list we create a "composite list" of notes and placeholders. The placeholders designate where headers will go, where the tag suggestions list will go, and where a message will go if there are no matching notes.

There were two issues with this structure:
 - it was difficult to type because of how its structure overlapped a `NoteEntity` but didn't have a key field for discriminating the type. everything works just fine but the type signatures become harder to write.
 - we were storing display information in the `data` property and that, as a render concern, wasn't necessary

Further, we were directly mutating `notes` which came from `filterNotes()` and the Redux state. We want to avoid directly mutating values we receive from other places in order to keep data flows clear and obvious.

In this patch we are replacing the object data type for placeholders with simpler string counterparts, moving the label text into the `render` function, and exchanging a mutating operation for a non-mutating one.

This is part of broader work to separate modifying the search parameters from
actually filtering the notes and was originally created as part of an
exploratory PR #1807.
@dmsnell dmsnell force-pushed the refactor/note-list-composite-structure branch from 9313cd8 to 8bfc50f Compare January 6, 2020 18:27
@dmsnell
Copy link
Member Author

dmsnell commented Jan 6, 2020

Updated to rebase RELEASE-NOTES with recent changes from develop

@dmsnell dmsnell merged commit b3b1603 into develop Jan 6, 2020
@dmsnell dmsnell deleted the refactor/note-list-composite-structure branch January 6, 2020 19:04
@codebykat codebykat added this to the 1.14 milestone Dec 23, 2020
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.

3 participants