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

Standardizing IconField implementation across the app #47196

Merged
merged 11 commits into from
Oct 7, 2019

Conversation

majagrubic
Copy link
Contributor

@majagrubic majagrubic commented Oct 3, 2019

Summary

Related to: #46450.
This PR:

  • adds a new FieldIcon component based on the previous FieldNameIcon component to be used across the app
  • creates a new LensFieldIcon component as a lightweight wrapper around the component around the FieldIcon created in previous step
  • makes use of the new LensFieldIcon component in Lens components
  • makes use of the new FieldIcon component in other components

Cleanup of the old components is not done here and will be addressed in the subsequent PR.

Some screenshots with the new icon:

Lens

Before:
Screenshot 2019-10-03 at 11 01 47

After:
Screenshot 2019-10-03 at 10 56 57

Discover

Before:
Screenshot 2019-10-03 at 11 00 21

After:
Screenshot 2019-10-03 at 11 17 56

Graph

Before:
Screenshot 2019-10-03 at 11 02 24

After:
Screenshot 2019-10-03 at 10 57 26

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@majagrubic majagrubic requested a review from a team October 3, 2019 09:46
@majagrubic majagrubic requested a review from a team as a code owner October 3, 2019 09:46
@majagrubic majagrubic changed the title Maja/fix 46450 Standardizing IconField implementation across the app Oct 3, 2019
@flash1293 flash1293 self-requested a review October 3, 2019 10:01
@majagrubic majagrubic added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 3, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@majagrubic majagrubic added Feature:Lens release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0 labels Oct 3, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

This looks mostly good to me, had some smaller comments that you could look into.

import { FieldIcon } from '../../../../../../src/plugins/kibana_react/public';
import { DataType } from '../types';

function stringToNum(s: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to get ride of this complete logic - the typeToEuiIconMap in src/plugins/kibana_react/public/icon/field_icon.tsx should provide the same information so we don't have to do this workaround here anymore with looking up an icon from a field type and then using it as an index for the color palette.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in FieldItem to determine the color for BarSeries:

const expectedColor = getColorForDataType(field.type);

I think leaving the function inside lens_field_icon makes sense, but just change it so that it uses typeToEuiIconMap internally.

export function LensFieldIcon({ type }: { type: DataType }) {
const classes = classNames(
'lnsFieldListPanel__fieldIcon',
`lnsFieldListPanel__fieldIcon--${type}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this didn't change in this PR but the lnsFieldListPanel__fieldIcon--${type} class name is not referenced anywhere (only in tests), so it makes sense to remove it.

@@ -24,7 +24,7 @@ interface IconMapEntry {
icon: string;
color: string;
}
interface FieldNameIconProps {
interface FieldIconProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also call the folder field_icon together with the component? I'm sure it's not done consistently across the app but if the folder mainly exports a single component it should be called like the component

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, great work 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@majagrubic majagrubic merged commit 79b63e0 into elastic:master Oct 7, 2019
@majagrubic majagrubic deleted the maja/fix-46450 branch October 7, 2019 08:36
majagrubic pushed a commit to majagrubic/kibana that referenced this pull request Oct 7, 2019
* Moving FieldNameIcon implementation to FieldIcon implementation in kibana_react directory

* Adding LensFieldIcon implementation around the new FieldIcon to be used in Lens components

* Applying new LensFieldIcon in the Lens components

* Applying new FieldIcon component in Graph components

* Applying new FieldIcon component in the rest of the components

* Adding missing type to lens field icon

* adding missing type

* Addressing PR comments
majagrubic pushed a commit that referenced this pull request Oct 7, 2019
* Moving FieldNameIcon implementation to FieldIcon implementation in kibana_react directory

* Adding LensFieldIcon implementation around the new FieldIcon to be used in Lens components

* Applying new LensFieldIcon in the Lens components

* Applying new FieldIcon component in Graph components

* Applying new FieldIcon component in the rest of the components

* Adding missing type to lens field icon

* adding missing type

* Addressing PR comments
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 7, 2019
… into console-token-iterator

* 'console-token-iterator' of github.com:jloleysens/kibana: (184 commits)
  [functional/services] update webdriver lib and types (elastic#47381)
  Standardizing IconField implementation across the app (elastic#47196)
  Move ui/value_suggestions ⇒ NP data plugin (elastic#45762)
  Remove ui/persisted_log - Part 2 (elastic#47236)
  Update gulp related packages (elastic#47421)
  Update dependency idx to ^2.5.6 (elastic#47399)
  try running fewer jobs in parallel on the same worker (elastic#47403)
  Update webpack related packages (elastic#47402)
  Update jsonwebtoken related packages (elastic#47400)
  Update gulp related packages (major) (elastic#46665)
  Update dependency prettier to ^1.18.2 (elastic#47340)
  Update dependency @types/puppeteer to ^1.20.1 (elastic#47339)
  Update dependency @elastic/elasticsearch to ^7.4.0 (elastic#47338)
  Update dependency tar-fs to ^1.16.3 (elastic#47341)
  [Code] Code Integrator Component (elastic#47180)
  [Canvas][i18n] Sidebar (elastic#46090)
  Generate uuid in task Manager as Kibana uuid may not yet have been initialised
  [Code] Embedded Code Snippet Component (elastic#47183)
  Revert "Add pipeline for flaky test runner job (elastic#46740)"
  SearchSource: fix docvalue_fields and fields intersection logic (elastic#46724)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants