Skip to content

[147] label tree#196

Merged
tomusdrw merged 18 commits intoFluffyLabs:mainfrom
DrEverr:147-label-tree
Mar 18, 2025
Merged

[147] label tree#196
tomusdrw merged 18 commits intoFluffyLabs:mainfrom
DrEverr:147-label-tree

Conversation

@DrEverr
Copy link
Member

@DrEverr DrEverr commented Mar 10, 2025

Newer iteration to prior #176

@netlify
Copy link

netlify bot commented Mar 10, 2025

Deploy Preview for graypaper-reader ready!

Name Link
🔨 Latest commit f303e3c
🔍 Latest deploy log https://app.netlify.com/sites/graypaper-reader/deploys/67d9ece54324350008b849ef
😎 Deploy Preview https://deploy-preview-196--graypaper-reader.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@DrEverr DrEverr requested a review from tomusdrw March 10, 2025 11:28
@DrEverr
Copy link
Member Author

DrEverr commented Mar 10, 2025

focused less on display, more on functionality

Copy link
Member

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

What I'm still missing is adding the source as a prefix to all labels (to be able to easily disable all local or remote labels at once).

@DrEverr
Copy link
Member Author

DrEverr commented Mar 11, 2025

Looks pretty good!

What I'm still missing is adding the source as a prefix to all labels (to be able to easily disable all local or remote labels at once).

I still believe it's better not to add the local/ prefix by default. This provides more control and keeps the existing behavior intact. If everything starts with local/ and we switch from hasAllLabels to a hasSomeLabels logic, it essentially makes it impossible to hide any notes. You'd always have to disable local, which would disable all the notes.

@tomusdrw
Copy link
Member

Looks pretty good!
What I'm still missing is adding the source as a prefix to all labels (to be able to easily disable all local or remote labels at once).

I still believe it's better not to add the local/ prefix by default. This provides more control and keeps the existing behavior intact. If everything starts with local/ and we switch from hasAllLabels to a hasSomeLabels logic, it essentially makes it impossible to hide any notes. You'd always have to disable local, which would disable all the notes.

No, that's incorrect. local/ and remote/ prefix is added depending on the source of the note. Existing labels have nothing to do with it and should not be modified to include the prefix. It's purely a display thing.

Example:

  1. In local storage:
    Note1(labels=x/a, c), Note2(labels=x/b, d), Note3(labels=local)
  2. In remote source:
    Note4(labels=x/a, c), Note5(labels=x/b, d), Note6(labels=local).

If we load notes from both sources we should get the following labels tree structure:

local
  local/x
     local/x/a, local/x/b
  local/c
  local/d
  local/local
remote
  remote/x
   remote/x/a, remote/x/b
  remote/c
  remote/d
  remote/local
  1. If I want to disable all local labels, I just click local top-level-meta-label.
  2. If I want to hide Note3 I need to have local/local inactive
  3. If I want to hide Note6 I need to have remote/local inactive
  4. If I want to hide Note2 I need to have local/d AND local/x/b inactive (or the entire local/x obviously).

@DrEverr DrEverr requested a review from tomusdrw March 11, 2025 17:53
@tomusdrw
Copy link
Member

Yes! I think it works as I expected now, if you could add the source prefix to the labels displayed next to notes it would be great (i.e. on top I see local/abc, yet under the note it's just abc).

Other than that I think it's good, thank you for bearing with me :)

@DrEverr
Copy link
Member Author

DrEverr commented Mar 12, 2025

I've made the last changes you suggested. Should I now focus on displaying labels as a tree, or are we leaving that for the designer?

@DrEverr DrEverr marked this pull request as ready for review March 12, 2025 07:26
@DrEverr
Copy link
Member Author

DrEverr commented Mar 17, 2025

IMO it's ready for merge. @tomusdrw WDYT?

@tomusdrw tomusdrw merged commit dad7c59 into FluffyLabs:main Mar 18, 2025
7 checks passed
@DrEverr DrEverr deleted the 147-label-tree branch March 19, 2025 06:51
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