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.

Sign up for GitHub

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

Jump to bottom

feat(axis): add visibility to tick style #374

Merged
merged 3 commits into from
Sep 19, 2019
Merged

feat(axis): add visibility to tick style #374

merged 3 commits into from
Sep 19, 2019

Conversation

nickofthyme
Copy link
Collaborator

Summary

Add visibility property to tickLineStyle in Theme.

BREAKING CHANGE: theme.axes.tickLineStyle.visible is now required (default base is false)

closes #330

Screen Recording 2019-09-12 at 01 20 PM

Checklist

  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@nickofthyme
feat(axis): add visibility to tick style
0f5d2b9 
Add visibility propterty to tickLineStyle in Theme. (default: false)

BREAKING CHANGE: `theme.axes.tickLineStyle.visible` is now required (default base is false)

closes elastic#330
@nickofthyme nickofthyme added enhancement New feature or request :axis Axis related issue labels Sep 12, 2019
@codecov-io
Copy link

codecov-io commented Sep 12, 2019 •
edited
Loading

Codecov Report

Merging #374 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #374   +/-   ##
=======================================
  Coverage   98.27%   98.27%           
=======================================
  Files          38       38           
  Lines        2784     2784           
  Branches      657      657           
=======================================
  Hits         2736     2736           
  Misses         44       44           
  Partials        4        4
Impacted Files Coverage Δ
src/utils/themes/light_theme.ts 100% <ø> (ø) ⬆️
src/utils/themes/dark_theme.ts 100% <ø> (ø) ⬆️
src/utils/themes/theme.ts 100% <ø> (ø) ⬆️

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 600f8e3...6eda1fa. Read the comment docs.

@markov00
Copy link
Member

Can we have the default visibility to true, and change the default to false when we are going to implement a better default style?

@nickofthyme
Copy link
Collaborator Author

@markov00 I set the default to false as that is the style EUI is suggesting. Are you saying to set the default to true and then convert to false later?

@markov00
Copy link
Member

Yes, I would like to have a single PR that adjust the default theme to a better one. For example in this case hide the ticks but also reduce the tick size to 0 for example. Together with some better defaults of style for the axis colors, grid style, line annotations etc

markov00
markov00 requested changes Sep 18, 2019
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Seems that you have rebased on top of an older version of master because the changes remove part of the fixes on current master.

package.json Outdated
@@ -1,7 +1,7 @@
{
"name": "@elastic/charts",
"description": "Elastic-Charts data visualization library",
"version": "12.0.2",
"version": "12.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

we cannot go back

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I didn't see that, must have been a bad merge. Fix in 6eda1fa

CHANGELOG.md Outdated
### Bug Fixes

* **theme:** fix grid position check ([#373](https://github.com/elastic/elastic-charts/issues/373)) ([af4805f](https://github.com/elastic/elastic-charts/commit/af4805f)), closes [#372](https://github.com/elastic/elastic-charts/issues/372)

Copy link
Member

Choose a reason for hiding this comment

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

Why does changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix in 6eda1fa

@markov00 markov00 self-requested a review September 19, 2019 08:29
markov00
markov00 approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Code LGTM

@markov00
Copy link
Member

jenkins, test this please

@nickofthyme nickofthyme merged commit 265a6bb into elastic:master Sep 19, 2019
@nickofthyme nickofthyme deleted the feat/hide-ticks branch September 19, 2019 18:02
markov00 pushed a commit that referenced this pull request Sep 19, 2019
@semantic-release-bot
chore(release): 13.0.0 [skip ci]
e1b0cd6 
# [13.0.0](v12.1.0...v13.0.0) (2019-09-19)

### Features

* **axis:** add visibility to tick style ([#374](#374)) ([265a6bb](265a6bb)), closes [#330](#330)

### BREAKING CHANGES

* **axis:** `theme.axes.tickLineStyle.visible` is now required (default base is false)
@markov00
Copy link
Member

🎉 This PR is included in version 13.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Sep 19, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
@semantic-release-bot
chore(release): 13.0.0 [skip ci]
741481b 
# [13.0.0](elastic/elastic-charts@v12.1.0...v13.0.0) (2019-09-19)

### Features

* **axis:** add visibility to tick style ([opensearch-project#374](elastic/elastic-charts#374)) ([29e9e1e](elastic/elastic-charts@29e9e1e)), closes [opensearch-project#330](elastic/elastic-charts#330)

### BREAKING CHANGES

* **axis:** `theme.axes.tickLineStyle.visible` is now required (default base is false)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markov00 markov00 markov00 approved these changes

@monfera monfera Awaiting requested review from monfera

Assignees
No one assigned
Labels
:axis Axis related issue enhancement New feature or request released Issue released publicly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove tick lines
3 participants
@nickofthyme @codecov-io @markov00