Skip to content

replace isNodeTitle property with corresponding tag and extend tag utilities#237

Open
Eddykasp wants to merge 1 commit into
mainfrom
mka/nodeTitleTag
Open

replace isNodeTitle property with corresponding tag and extend tag utilities#237
Eddykasp wants to merge 1 commit into
mainfrom
mka/nodeTitleTag

Conversation

@Eddykasp

Copy link
Copy Markdown
Member

depends on kieler/KLighD#234

@Eddykasp Eddykasp added the requires server change This change is linked to some change on the server side implementation in KLighD label Jul 24, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces the use of the isNodeTitle property with a corresponding semantic filter tag and extends tag utility functions to support this change. The refactoring moves away from direct property access to a more structured tag-based approach for identifying node titles.

  • Replaces direct property access rendering.properties['klighd.isNodeTitle'] with findTag(rendering, 'isNodeTitle') calls
  • Adds a new findTag utility function to search for semantic filter tags by ID
  • Updates documentation comments to reflect the change from property to tag-based approach

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/klighd-core/src/views-rendering.tsx Updates rendering logic to use findTag instead of direct property access for isNodeTitle checks
packages/klighd-core/src/titles/title-storage.ts Updates documentation comment to reflect change from property to tag
packages/klighd-core/src/filtering/util.ts Adds PropertyHolder type and findTag utility function for semantic tag lookup
Comments suppressed due to low confidence (1)

packages/klighd-core/src/filtering/util.ts:125

  • [nitpick] The type name 'PropertyHolder' is generic and doesn't clearly convey its purpose for semantic filtering. Consider renaming it to 'SemanticFilterElement' or 'TaggableElement' to better reflect its role in the semantic filtering system.
export type PropertyHolder = {

}

/**
* Type to check that properties exist.

Copilot AI Jul 24, 2025

Copy link

Choose a reason for hiding this comment

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

The comment 'Type to check that properties exist' is unclear and doesn't explain the type's purpose. It should describe that this type represents objects that can hold semantic filter properties, not just any properties.

Suggested change
* Type to check that properties exist.
* Represents objects that hold semantic filter-related properties.
* These properties are used to define and retrieve semantic filter rules and tags
* for graph elements in the filtering process.

Copilot uses AI. Check for mistakes.

@soerendomroes soerendomroes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the copilot suggestion is valid (it seems to be) I would address it, but otherwise lgtm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires server change This change is linked to some change on the server side implementation in KLighD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants