Skip to content
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

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

joshuarrrr
Copy link
Member

@joshuarrrr joshuarrrr commented Apr 25, 2022

Description

  • fix typo in iconType for replace_panel action
  • change iconType types from string to EuiIconType
  • use icon instead of iconType for svg paths

Issues Resolved

#1475

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

- 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>
@joshuarrrr joshuarrrr requested a review from a team as a code owner April 25, 2022 23:32
@codecov-commenter
Copy link

Codecov Report

Merging #1496 (f35280b) into main (e0f394e) will not change coverage.
The diff coverage is 100.00%.

@@           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           
Impacted Files Coverage Δ
src/core/public/chrome/nav_links/nav_link.ts 66.66% <ø> (ø)
...blic/application/actions/add_to_library_action.tsx 54.54% <ø> (ø)
.../public/application/actions/clone_panel_action.tsx 68.57% <ø> (ø)
...public/application/actions/expand_panel_action.tsx 47.36% <ø> (ø)
...application/actions/unlink_from_library_action.tsx 54.54% <ø> (ø)
...embeddable/public/lib/actions/edit_panel_action.ts 48.71% <ø> (ø)
...header/panel_actions/add_panel/add_panel_action.ts 100.00% <ø> (ø)
..._actions/customize_title/customize_panel_action.ts 100.00% <ø> (ø)
...panel_header/panel_actions/inspect_panel_action.ts 83.33% <ø> (ø)
.../panel_header/panel_actions/remove_panel_action.ts 92.30% <ø> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0f394e...f35280b. Read the comment docs.

Copy link
Member

@ashwin-pc ashwin-pc left a 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,
Copy link
Member

@ashwin-pc ashwin-pc Apr 26, 2022

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

Copy link
Member Author

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>
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM!

@tmarkley
Copy link
Contributor

tmarkley commented Apr 27, 2022

We'll need to decide if we want to fit this in between 2.0 RC1 and GA or wait until 2.1.

@ashwin-pc
Copy link
Member

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

@ashwin-pc ashwin-pc merged commit ef3a9c0 into opensearch-project:main Apr 27, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to pending failed:

The process '/usr/bin/git' failed with exit code 128

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 base branch is pending and the compare/head branch is backport/backport-1496-to-pending.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.x failed:

The process '/usr/bin/git' failed with exit code 1

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 base branch is 1.x and the compare/head branch is backport/backport-1496-to-1.x.

opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 28, 2022
* 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)
@opensearch-trigger-bot
Copy link
Contributor

The backport to 1.3 failed:

The process '/usr/bin/git' failed with exit code 1

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 base branch is 1.3 and the compare/head branch is backport/backport-1496-to-1.3.

tmarkley pushed a commit to tmarkley/OpenSearch-Dashboards that referenced this pull request Apr 28, 2022
- 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>
tmarkley pushed a commit to tmarkley/OpenSearch-Dashboards that referenced this pull request Apr 28, 2022
- 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>
tmarkley pushed a commit that referenced this pull request Apr 29, 2022
- 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>
tmarkley pushed a commit that referenced this pull request Apr 29, 2022
- 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>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 29, 2022
* 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)
tmarkley pushed a commit that referenced this pull request Apr 29, 2022
- 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>
(cherry picked from commit ef3a9c0)
@tmarkley tmarkley added the bug Something isn't working label May 3, 2022
tmarkley pushed a commit that referenced this pull request May 3, 2022
- 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>
(cherry picked from commit ef3a9c0)
@tmarkley tmarkley mentioned this pull request May 6, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants