Skip to content

refactor(devtools): restyle the signal details panel #62681

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

Closed
wants to merge 2 commits into from

Conversation

hawkgs
Copy link
Member

@hawkgs hawkgs commented Jul 17, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature
  • Refactoring (no functional changes, no api changes)

What is the new behavior?

Restyle the details panel to match and be inline with the existing UI/style.

Changes:

  • The signal details panel is moved in a separate component (signal-details) for easier stylization.
  • signals-value-tree is moved in a separated folder with separate a BUILD file.
  • Both signal-details and signals-value-tree are restyled.
Light modeDark mode
Screenshot 2025-07-17 at 12 29 18 Screenshot 2025-07-17 at 12 29 06

The design resembles the initial wireframes.

@hawkgs hawkgs requested review from dgp1130 and milomg July 17, 2025 09:58
@hawkgs hawkgs added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release area: devtools labels Jul 17, 2025
@ngbot ngbot bot added this to the Backlog milestone Jul 17, 2025
@hawkgs hawkgs force-pushed the devtools/signals-graph-details branch from b368d83 to 9d8e40b Compare July 17, 2025 10:02
Comment on lines +22 to +23
@if (node.preview.preview === 'undefined') {
<div class="value">undefined</div>
Copy link
Member Author

Choose a reason for hiding this comment

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

@milomg, does this make sense? Using just "undefined" instead of "Computed signal value is undefined"?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I prefer this

@hawkgs hawkgs force-pushed the devtools/signals-graph-details branch from e3b8703 to 7e7e928 Compare July 17, 2025 11:53
Copy link
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

No concerns on my side, just consider adding tests for any new functionality.

@hawkgs hawkgs added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 18, 2025
@ngbot
Copy link

ngbot bot commented Jul 18, 2025

I see that you just added the action: merge label, but the following checks are still failing:
    failure conflicts with base branch "main"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@hawkgs hawkgs added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jul 18, 2025
hawkgs added 2 commits July 18, 2025 10:17
@hawkgs hawkgs force-pushed the devtools/signals-graph-details branch from 7e7e928 to fee8b07 Compare July 18, 2025 07:17
@hawkgs
Copy link
Member Author

hawkgs commented Jul 18, 2025

No functional changes.

@hawkgs hawkgs added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 18, 2025
@kirjs
Copy link
Contributor

kirjs commented Jul 18, 2025

This PR was merged into the repository by commit d3b770d.

The changes were merged into the following branches: main

@kirjs kirjs closed this in d3b770d Jul 18, 2025
@hawkgs hawkgs deleted the devtools/signals-graph-details branch July 18, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: devtools target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants