Icon refactoring and Summary spacing fix #6951
Conversation
| @@ -35,7 +35,7 @@ describe('scoreNodes', () => { | |||
| color: 'any', | |||
| priority: 1, | |||
| icon: WarningTriangleIcon, | |||
| class: 'any' | |||
| className: 'any' | |||
There was a problem hiding this comment.
Renamed to className to be consistent with React className field.
| @@ -51,7 +51,7 @@ | |||
| "cypress:delete:reports": "rm cypress/results/* || true", | |||
| "cypress:combine:reports": "jrm cypress/results/combined-report.xml \"cypress/results/*.xml\"", | |||
| "lint": "eslint --ext js,ts,tsx src", | |||
| "lint:precommit": "eslint -c .eslintrc_precommit $(git diff --staged --name-only HEAD | grep -E '\\.tsx?$' | cut -d'/' -f2-)", | |||
| "lint:precommit": "eslint -c .eslintrc_precommit $(git diff --staged --name-only HEAD --diff-filter=AM | grep -E '\\.tsx?$' | cut -d'/' -f2-)", | |||
There was a problem hiding this comment.
Filter added to apply precommit lint only to added and modified files (it was throwing lint error because of deleted file)
There was a problem hiding this comment.
I like these author-written review comments, it's helpful without having to add comments in the code.
| import { healthIndicatorStyle } from '../../styles/HealthStyle'; | ||
|
|
||
| interface HealthIndicatorProps { | ||
| health?: H.Health; | ||
| id: string; | ||
| size?: Size; |
There was a problem hiding this comment.
Health indicator size should be aligned wiith current text next to indicator (font-size). For this reason I have removed specific size property.
There was a problem hiding this comment.
createIcon function moved to config/KialiIcon file
There was a problem hiding this comment.
It's a bit hard to find the core changes within all of the linting changes, but I'm happy that you touched so many files so that other people (like me) didn't have to do all of the fixes :-) I definitely like the centralized Icon management, it should reduce surprises in icon rendering across the UI. In the code changes you can already see places where we were able to remove single-point styling, which is always good.
| @@ -51,7 +51,7 @@ | |||
| "cypress:delete:reports": "rm cypress/results/* || true", | |||
| "cypress:combine:reports": "jrm cypress/results/combined-report.xml \"cypress/results/*.xml\"", | |||
| "lint": "eslint --ext js,ts,tsx src", | |||
| "lint:precommit": "eslint -c .eslintrc_precommit $(git diff --staged --name-only HEAD | grep -E '\\.tsx?$' | cut -d'/' -f2-)", | |||
| "lint:precommit": "eslint -c .eslintrc_precommit $(git diff --staged --name-only HEAD --diff-filter=AM | grep -E '\\.tsx?$' | cut -d'/' -f2-)", | |||
There was a problem hiding this comment.
I like these author-written review comments, it's helpful without having to add comments in the code.
|
|
||
| type ReduxProps = { | ||
| type ReduxStateProps = { |
There was a problem hiding this comment.
Not sure splitting ReduxProps is necessary but it's fine, it's still clear that these are connected props.
There was a problem hiding this comment.
The split is not really needed in a functional point of view. It is just a way to fix lint issue (define return type of redux methods):
const mapStateToProps = (state: KialiAppState): ReduxStateProps
const mapDispatchToProps = (dispatch: KialiDispatch): ReduxDispatchProps
Since all fields are mandatory you cannot just assign ReduxProps to both methods
Describe the change
Currently in Kiali there are multiple ways to create an icon:
Having many ways creates confusion to developers and can have different structure (SVG icon vs PF icon) that affects CSS styles.
For this reason I think it is good to have only one structure through Kiali application (PF icon) and simplify icon creation:
From now the icon criteria should be:
Icons used in graphs (config/Icons for cytoscape and styleGroup and styleNode for PF) are excluded from this refactoring.
More stuff done in this PR:
from HTML)
Before:
After:
Before:
After:
Before:
After:
Before:
After:
Issue reference
Fixes #6946