-
Notifications
You must be signed in to change notification settings - Fork 934
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
fix(actions): Better type checks for icons #1496
fix(actions): Better type checks for icons #1496
Conversation
- fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin Signed-off-by: Josh Romero <rmerqg@amazon.com>
Codecov Report
@@ Coverage Diff @@
## main #1496 +/- ##
=======================================
Coverage 68.09% 68.09%
=======================================
Files 3072 3072
Lines 59032 59032
Branches 8928 8928
=======================================
Hits 40198 40198
Misses 16647 16647
Partials 2187 2187
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small change about casting the type as const
instead of EuiIconType
. The rest of the PR looks good!
@@ -41,7 +42,7 @@ const availableApps = new Map([ | |||
id: 'app2', | |||
order: -10, | |||
title: 'App 2', | |||
euiIconType: 'canvasApp', | |||
euiIconType: 'canvasApp' as EuiIconType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of casting it as EuiIconType
can we instead cast it as a const
i.e.
euiIconType: 'canvasApp' as const,
This way we dont go back to the same issue where we use any string and accidentally use a value not available in eui
The same goes for every instance in the PR that you are casting the iconType as EuiIconType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I forgot that those are really assertions, not casts 😢 . The only one that makes sense to assert is the one test where we are intentionally using a made-up string rather than a valid icon name. Fixed.
Signed-off-by: Josh Romero <rmerqg@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
We'll need to decide if we want to fit this in between 2.0 RC1 and GA or wait until 2.1. |
This PR only improves the types used for icons. That should not cause a breaking change an can be safely introduced between RC1 and GA |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-pending pending
# Navigate to the new working tree
cd .worktrees/backport-pending
# Create a new branch
git switch --create backport/backport-1496-to-pending
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ef3a9c07a07ffb7f7bfa662d29c88ed487b7e187
# Push it to GitHub
git push --set-upstream origin backport/backport-1496-to-pending
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-pending Then, create a pull request where the |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-1496-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ef3a9c07a07ffb7f7bfa662d29c88ed487b7e187
# Push it to GitHub
git push --set-upstream origin backport/backport-1496-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x Then, create a pull request where the |
* fix(actions): Better type checks for icons - fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin Signed-off-by: Josh Romero <rmerqg@amazon.com> * fix(actions): Remove unnecessary icon assertions Signed-off-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit ef3a9c0)
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-1496-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ef3a9c07a07ffb7f7bfa662d29c88ed487b7e187
# Push it to GitHub
git push --set-upstream origin backport/backport-1496-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3 Then, create a pull request where the |
- fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin - remove unnecessary icon assertions Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Tommy Markley <markleyt@amazon.com>
- fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin - remove unnecessary icon assertions Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Tommy Markley <markleyt@amazon.com>
- fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin - remove unnecessary icon assertions Resolves #1475 Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Tommy Markley <markleyt@amazon.com>
- fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin - remove unnecessary icon assertions Resolves #1475 Signed-off-by: Josh Romero <rmerqg@amazon.com> Co-authored-by: Tommy Markley <markleyt@amazon.com>
* fix(actions): Better type checks for icons - fix typo in iconType for `replace_panel` action - change iconType types from `string` to `EuiIconType` - use `icon` instead of `iconType` for svg pathseslin Signed-off-by: Josh Romero <rmerqg@amazon.com> * fix(actions): Remove unnecessary icon assertions Signed-off-by: Josh Romero <rmerqg@amazon.com> (cherry picked from commit ef3a9c0)
Description
replace_panel
actionstring
toEuiIconType
icon
instead oficonType
for svg pathsIssues Resolved
#1475
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr