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

move YAxis inside Axis #8253

Merged
merged 6 commits into from
Sep 14, 2016
Merged

Conversation

ppisljar
Copy link
Member

moving YAxis inside Axis

@ppisljar ppisljar changed the title Vislibaxis/2 move YAxis inside Axis Sep 13, 2016
@epixa epixa added the Feature:Vislib Vislib chart implementation label Sep 13, 2016
this.expandLastBucket = args.expandLastBucket == null ? true : args.expandLastBucket;
this._attr = _.defaults(args._attr || {});
this.scale = null;
this.domain = [args.yMin, args.yMax];
Copy link
Contributor

Choose a reason for hiding this comment

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

is domain only used by the yAxis? It would make sense for xAxis too right?

Copy link
Member Author

Choose a reason for hiding this comment

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

its only used for yAxis ... the code in this PR is pretty bad as i only joined the 2 files to make it clear how i progressed. yAxis and xAxis have many functions that actually do same/similar thing but are named differently and sometimes work in different ways (like on one axis you are supposed to pass a scale to function which returns domain and on the other one you are supposed to pass in limits and then apply the return value on scale ... i wouldn't worry about all that in this PR ... its just to show steps of progress ... so putting all the functions from both files into one ... then we can clearly see in the diffs what have been changed from here on ...

@spalger
Copy link
Contributor

spalger commented Sep 14, 2016

I like this progression, and I'm kind of shocked how well the y-axis works considering.

I think fixing the way it calculates the width could be enough to fix the current issue

image

@spalger
Copy link
Contributor

spalger commented Sep 14, 2016

Also, feel free to respond to any and all feedback with "I'll get there in a new pr" or something if you would rather push it in later.

@spalger spalger mentioned this pull request Sep 14, 2016
@ppisljar
Copy link
Member Author

hmm ... not sure how i was testing, i was under impression that it works ok ...

@spalger
Copy link
Contributor

spalger commented Sep 14, 2016

Well, I'm fine with it as an intermediate step :)

@spalger spalger merged commit f168565 into elastic:vislib-axis-refactor Sep 14, 2016
@ppisljar
Copy link
Member Author

2fast :) i was trying to push in some changes based on your comments + fix so this actually works as it should

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Vislib Vislib chart implementation review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants