Skip to content

[Chore] Minor CSS changes for error overlay in feature tree #6977

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nadr0
Copy link
Contributor

@nadr0 nadr0 commented May 15, 2025

  • Display the error pane if there are errors or the operation list is present
    • The problem was when you refresh the page you don't have the feature tree because it hasn't executed once so the error header shows up but not the error background this is confusing
  • Changed the font coloring and size for the error header
  • Rounded the view error button it was too square

image

Copy link

vercel bot commented May 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2025 6:25pm

Comment on lines 51 to 57
[NetworkHealthState.Ok]: {
icon: 'text-succeed-80 dark:text-succeed-10',
bg: 'bg-succeed-10/30 dark:bg-succeed-80/50',
icon: 'text-destroy-80 dark:text-destroy-10',
bg: 'bg-destroy-10 dark:bg-destroy-80',
},
[NetworkHealthState.Weak]: {
icon: 'text-warn-80 dark:text-warn-10',
bg: 'bg-warn-10 dark:bg-warn-80/80',
icon: 'text-destroy-80 dark:text-destroy-10',
bg: 'bg-destroy-10 dark:bg-destroy-80',
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to NetworkHealthState.Ok and NetworkHealthState.Weak appear to be using error styling (destroy colors) instead of their appropriate semantic colors. This would make all network states visually identical regardless of their actual status.

For proper visual differentiation:

  • Ok state should maintain success colors (text-succeed-80/dark:text-succeed-10)
  • Weak state should maintain warning colors (text-warn-80/dark:text-warn-10)

These changes seem unrelated to the PR's stated purpose of fixing the error overlay in the feature tree, and would negatively impact the user's ability to distinguish between different network health states.

Suggested change
[NetworkHealthState.Ok]: {
icon: 'text-succeed-80 dark:text-succeed-10',
bg: 'bg-succeed-10/30 dark:bg-succeed-80/50',
icon: 'text-destroy-80 dark:text-destroy-10',
bg: 'bg-destroy-10 dark:bg-destroy-80',
},
[NetworkHealthState.Weak]: {
icon: 'text-warn-80 dark:text-warn-10',
bg: 'bg-warn-10 dark:bg-warn-80/80',
icon: 'text-destroy-80 dark:text-destroy-10',
bg: 'bg-destroy-10 dark:bg-destroy-80',
[NetworkHealthState.Ok]: {
icon: 'text-succeed-80 dark:text-succeed-10',
bg: 'bg-succeed-10 dark:bg-succeed-80',
},
[NetworkHealthState.Weak]: {
icon: 'text-warn-80 dark:text-warn-10',
bg: 'bg-warn-10 dark:bg-warn-80',
},

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +164 to +167
className={`absolute inset-0 p-2 ${
operationList.length ||
(parseErrors.length > 0 &&
`bg-destroy-10/40 dark:bg-destroy-80/40`)
Copy link
Contributor

Choose a reason for hiding this comment

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

The conditional logic for applying the background class needs correction. The current implementation with || followed by a string template literal will cause unexpected behavior - if operationList.length is truthy but not a string, it will be used as the class name instead of the intended background class.

Consider refactoring to use a ternary operator:

className={`absolute inset-0 p-2 ${
  (operationList.length || parseErrors.length > 0) 
    ? 'bg-destroy-10/40 dark:bg-destroy-80/40' 
    : ''
}`}

This ensures the background class is only applied when either condition is met, and always returns a string value.

Suggested change
className={`absolute inset-0 p-2 ${
operationList.length ||
(parseErrors.length > 0 &&
`bg-destroy-10/40 dark:bg-destroy-80/40`)
className={`absolute inset-0 p-2 ${
(operationList.length || parseErrors.length > 0)
? 'bg-destroy-10/40 dark:bg-destroy-80/40'
: ''
}`}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

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.

1 participant