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

[Maps] Enable borders for icon symbols #43066

Merged
merged 7 commits into from
Aug 12, 2019

Conversation

nickpeihl
Copy link
Member

Fixes #42949. This extends the Border Width and Border Color options for icon symbols in the Maps app.

localhost_5601_azg_app_maps

@nickpeihl nickpeihl requested a review from nreese August 9, 2019 22:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

Adding borders to icons looks great and really highlights all of the icon details. Thanks for putting this PR together.

Can you also update the docs, https://github.com/elastic/kibana/blob/master/docs/maps/vector-style-properties.asciidoc, adding Border color and Border width to the icon section?

}

componentDidMount() {
this._isMounted = true;
this._loadSymbol(this.props.symbolId, this.props.fill);
this._loadSymbol(this.props.symbolId, this.props.fill, this.props.stroke, this.props.strokeWidth);
Copy link
Contributor

Choose a reason for hiding this comment

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

Update SymbolIcon.propTypes definition at end of file to include stroke and strokeWidth

</Fragment>
);
} else {
const lineColor = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Since lineColor and lineWidth are now constant, their definition can just be moved directly into the return block and lineColor and lineWidth variables are no longer needed.

@@ -185,9 +170,15 @@ export class VectorStyleEditor extends Component {
{this._renderFillColor()}
<EuiSpacer size="m" />

{lineColor}
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fragment wrapper is not needed here


{lineWidth}
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fragment wrapper is not needed here

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm with green CI
code review, tested in chrome

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nickpeihl nickpeihl merged commit 3503aa1 into elastic:master Aug 12, 2019
nickpeihl added a commit to nickpeihl/kibana that referenced this pull request Aug 12, 2019
* [Maps] Enable borders on icons

* [Maps] Enable borders on icons

* Review feedback

* Remove unnecessary Fragment
@nickpeihl nickpeihl deleted the maps-add-borders-icons branch August 12, 2019 18:23
nickpeihl added a commit that referenced this pull request Aug 12, 2019
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 13, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (27 commits)
  [ML] Data Frames: Analytics job creation. (elastic#43102)
  [Vis Default editor] Fix issue with Rollup (elastic#42430)
  [Vis: Default editor] EUIficate Markdown tab (elastic#42677)
  [New Platform Migration Phase I]: update dateHisogramInterval & parseEsInterval imports (elastic#42917)
  [Infra UI] Add AWS metrics to node detail page (elastic#42153)
  update apm index pattern (elastic#43106)
  [SIEM] Toggle Column / Code Coverage and Cypress (elastic#42766)
  skip failing test (elastic#43163)
  [code] Add option to turn the go dependency download on/off. (elastic#43096)
  disable visual regression jobs
  Removed dead code (elastic#42774)
  fixes csv export of saved searches that have _source field (elastic#43123)
  Export missing Context types (elastic#43051)
  Update dependency supports-color to v7 (elastic#43064)
  switch to icon type string instead of node (elastic#43111)
  [Maps] Enable borders for icon symbols (elastic#43066)
  [ftr] enable visualRegression jobs (elastic#42989)
  [ML] Converting single to multi metric job (elastic#42532)
  fix(NA): dont clean dll module if it is a package json file (elastic#42904)
  [Logs UI] Add link from the sample web logs to the Logs UI (elastic#42635)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Add "Border Width" range selector to icons
3 participants