Skip to content

[147] hierarchical labels#176

Closed
DrEverr wants to merge 35 commits intoFluffyLabs:mainfrom
DrEverr:147-hierarchical-labels
Closed

[147] hierarchical labels#176
DrEverr wants to merge 35 commits intoFluffyLabs:mainfrom
DrEverr:147-hierarchical-labels

Conversation

@DrEverr
Copy link
Member

@DrEverr DrEverr commented Feb 18, 2025

#147

  • add hierarchical labels
  • toggle groups of labels

@netlify
Copy link

netlify bot commented Feb 18, 2025

Deploy Preview for graypaper-reader ready!

Name Link
🔨 Latest commit fffa90b
🔍 Latest deploy log https://app.netlify.com/sites/graypaper-reader/deploys/67c4c9e0351743000844db1d
😎 Deploy Preview https://deploy-preview-176--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 and others added 8 commits February 19, 2025 10:14
@DrEverr DrEverr requested a review from tomusdrw February 20, 2025 13:42
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.

As discussed off-band, the PR needs a bit more work to align with the requirements from #147

@DrEverr
Copy link
Member Author

DrEverr commented Feb 25, 2025

Still as draft, because it doesn't re-trigger filteredNotes on label change.

@DrEverr DrEverr requested a review from tomusdrw February 26, 2025 10:06
Comment on lines +15 to +23
if (!("parent" in x && (x.parent === null || isLabel(x.parent)))) {
return false;
}
if (!("children" in x && Array.isArray(x.children))) {
return false;
}
if (!("notes" in x && Array.isArray(x.notes))) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

This should not be here. In local storage the notes should be stored in a flat format as currently and not as a tree.

Comment on lines +52 to +58
## Options

```
Usage: gp-links-check [options] <paths...>

Arguments:
paths Paths of files to be scanned. Supports glob patterns.
Copy link
Member

Choose a reason for hiding this comment

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

These changes are already on main, there is something funky happening in the PR. Can you please try to re-open it as a new one or clean up the history?

Comment on lines +56 to +62
if (parts.length === 1) {
if (parts[0] === LABEL_LOCAL || parts[0] === LABEL_REMOTE) {
root.notes.push(note);
continue;
}
}
addToTree(parts, note, root);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (parts.length === 1) {
if (parts[0] === LABEL_LOCAL || parts[0] === LABEL_REMOTE) {
root.notes.push(note);
continue;
}
}
addToTree(parts, note, root);
// if the label starts with `local` or `remote` just trim the first part.
if (parts[0] === LABEL_LOCAL || parts[0] === LABEL_REMOTE) {
parts.shift();
}
addToTree(parts, note, root);

I think this is more correct way to handle that case. I wonder however where does this special casing come from? Do we currently have notes with explicit local or remote label within them? I think it's totally fine if they were displayed as local/local - the user may remove that label if it was added there unnecessarily.

Copy link
Member Author

Choose a reason for hiding this comment

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

Our other mechanism continuously adds the ‘local’ label, resulting in ‘local/local’ on each page refresh.


export function Label({ label, prefix = "" }: { label: string; prefix?: string }) {
const backgroundColor = useMemo(() => labelToColor(label), [label]);
const backgroundColor = useMemo(() => labelToColor(label.split("/").pop() || ""), [label]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const backgroundColor = useMemo(() => labelToColor(label.split("/").pop() || ""), [label]);
const backgroundColor = useMemo(() => labelToColor(label), [label]);

Background color should depend on the whole "path" (i.e. remote label v0.4.5 should have different color than a local one with the same name)

Copy link
Member Author

Choose a reason for hiding this comment

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

it makes labels in notes and labels on the tree displays differently (w/ different color)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we should have full path in notes then.

Copy link
Member Author

Choose a reason for hiding this comment

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

full path displayed on tree?
eg.
local
local/a
local/a/b
local/a/c

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that we should always use full path for color and display the text-label depending on the context:

  1. In the tree we just display shorter label (since it's nested)
  2. In the note we display full label (perhaps without the source part)

<div
className="label-node-header"
onClick={() => onToggleLabel(label)}
onKeyUp={(e) => e.key === "Enter" && setExpanded(!expanded)}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onKeyUp={(e) => e.key === "Enter" && setExpanded(!expanded)}

that shouldn't be needed. The group should be expanded when it's active and folded when inactive.

);

function LabelNode({ label, onToggleLabel }: LabelLinkProps) {
const [expanded, setExpanded] = useState(true);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [expanded, setExpanded] = useState(true);
const expanded = label.isActive;

DrEverr and others added 2 commits March 2, 2025 21:57
Co-authored-by: Tomek Drwięga <tomusdrw@users.noreply.github.com>
@DrEverr DrEverr mentioned this pull request Mar 10, 2025
@DrEverr
Copy link
Member Author

DrEverr commented Mar 11, 2025

Continued in #196

@DrEverr DrEverr closed this Mar 11, 2025
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