Skip to content

feat: component graph node name toogle #797

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

Merged
merged 9 commits into from
Mar 5, 2025

Conversation

runyasak
Copy link
Contributor

πŸ”— Linked issue

resolves #694

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

I added the PascalCase component name to the selectedLabel field in extra as the selected display label, while keeping label as the current node label.

Next, I implemented DataSet for nodes, allowing node labels to be updated by their ID.

I also updated the playground to reproduce this enhancement. If this update is unnecessary, I’m happy to remove it as needed.

Screenshot

Screen.Recording.2568-02-27.at.01.01.43.mov

If you have any feedback or suggestions, please don’t hesitate to let me know.

Sorry, something went wrong.

@@ -99,9 +102,12 @@ const data = computed<Data>(() => {
const isGrayedOut = searchDebounced.value && !rel.id.toLowerCase().includes(searchDebounced.value.toLowerCase())
const label = path.split('/').splice(-1)[0].replace(/\.\w+$/, '')
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use an utility function to infer a better name from the path, instead of relying on the selected state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting! But I'm not sure what the best approach is to extract the component name from the path.

Could you review my function if I implement it like this?

function getComponentName(path: string) {
  const splitPath = path.split('/');
  const lastChunkPath = splitPath.splice(-1)[0].replace(/\.\w+$/, '');

  if (lastChunkPath === 'index') {
    return splitPath.splice(-2)[0].replace(/\.\w+$/, '')
  }

  return lastChunkPath
}

If my code is incorrect or if you have a better approach, please let me know.

@runyasak
Copy link
Contributor Author

runyasak commented Mar 2, 2025

@antfu I pushed my updated version using the getComponentName utility function, without relying on the selected state.

However, I’m not sure if this approach aligns with your preference.
Please let me know if you have any comments or suggestions, and I’ll update my code as soon as possible.

antfu added 2 commits March 5, 2025 15:55

Verified

This commit was signed with the committer’s verified signature.
antfu Anthony Fu

Verified

This commit was signed with the committer’s verified signature.
antfu Anthony Fu
@antfu
Copy link
Member

antfu commented Mar 5, 2025

I am not sure if I understand why we need to update the label name on select and reset back. Shouldn't we just give the correct name at the first place?

I updated the PR to change that. Hope that makes sense to you (let me know if not). Thanks for the PR

@antfu antfu merged commit 2eb2a37 into nuxt:main Mar 5, 2025
1 of 2 checks passed
@runyasak
Copy link
Contributor Author

runyasak commented Mar 5, 2025

@antfu Thank you for reviewing my code.

I am not sure if I understand why we need to update the label name on select and reset back. Shouldn't we just give the correct name at the first place?

I was just following the approach to create the toggle feature as described in the issue #694.
However, I also initially thought about providing the correct name from the start, just like you suggested.

Would you like me to create a new PR to implement this approach?
I can revert the old code and update it to directly set the correct name as the node label.

@antfu
Copy link
Member

antfu commented Mar 5, 2025

I already change that in 3b5fd26 (#797)

@runyasak
Copy link
Contributor Author

runyasak commented Mar 5, 2025

Thank you so much.
I'm looking for the next contribution. 😁

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.

feat: component graph node name toogle
2 participants