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

[Bug] fix missing discover context icon #1545

Merged

Conversation

joshuarrrr
Copy link
Member

Add back icon directive, which injects SVG for EUI icons

Signed-off-by: Josh Romero rmerqg@amazon.com

Description

Corrects rendering of discover context table row expansion buttons. The directive appears to have been removed when partially de-angularizing discover, without realizing the impact on the context doc table rendering.

Issues Resolved

fixes #1281

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

add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Nice! Is it worthwhile to add tests to ensure the icons are now loaded properly to prevent a regression in the future?

@joshuarrrr
Copy link
Member Author

Nice! Is it worthwhile to add tests to ensure the icons are now loaded properly to prevent a regression in the future?

Ideally, yes, absolutely.

The problem is I'm not sure there's a straightforward way to do that for these legacy views that are rendered with a mix of Angular and React. Specifically, the regression case we'd want to test is that we're dynamically inserting the icon SVG on any angular-defined <icon> state change so that

<icon ng-if="open" type="'arrowDown'" size="'s'" class="ng-scope"></icon>

becomes

<icon ng-if="open" type="'arrowDown'" size="'s'" class="ng-scope"><svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" class="euiIcon euiIcon--small" focusable="false" role="img" aria-hidden="true"><path fill-rule="non-zero" d="M13.069 5.157L8.384 9.768a.546.546 0 01-.768 0L2.93 5.158a.552.552 0 00-.771 0 .53.53 0 000 .759l4.684 4.61c.641.631 1.672.63 2.312 0l4.684-4.61a.53.53 0 000-.76.552.552 0 00-.771 0z"></path></svg></icon>

It probably makes more sense to spend some time investigating the functional tests in discover, to see if they provide enough coverage for the larger effort of finishing the migration from Angular to React.

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

   __   _____________  ___
  / /  / ___/_  __/  |/  /
 / /__/ (_ / / / / /|_/ / 
/____/\___/ /_/ /_/  /_/  

@tmarkley
Copy link
Contributor

tmarkley commented May 6, 2022

It probably makes more sense to spend some time investigating the functional tests in discover, to see if they provide enough coverage for the larger effort of finishing the migration from Angular to React.

@joshuarrrr I think that's reasonable. We'll need to address that with #1560.

@tmarkley tmarkley merged commit bd10227 into opensearch-project:main May 6, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
tmarkley pushed a commit that referenced this pull request May 6, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
@tmarkley tmarkley mentioned this pull request May 6, 2022
5 tasks
tmarkley pushed a commit that referenced this pull request May 10, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
tmarkley pushed a commit that referenced this pull request May 10, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
joshuarrrr added a commit to joshuarrrr/OpenSearch-Dashboards that referenced this pull request May 11, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
joshuarrrr added a commit that referenced this pull request Jun 2, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
tmarkley pushed a commit that referenced this pull request Jun 3, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit bd10227)
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 8, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jun 16, 2022
add back icon directive, which injects SVG

Signed-off-by: Josh Romero <rmerqg@amazon.com>
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.

[BUG] Document table expand icon is invisible on Discover Context page
4 participants