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

Highcharts/4 seperate axes #5

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

MarkusBarstad
Copy link
Contributor

No description provided.

@MarkusBarstad MarkusBarstad self-assigned this Aug 24, 2022
Copy link

@K009 K009 left a comment

Choose a reason for hiding this comment

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

Good job Markus! The exercise is done correct. However I've found some things that can be improved. Please take a look at them.

@MarkusBarstad
Copy link
Contributor Author

Thank you for good feedback, I will look at it now! :)

@MarkusBarstad
Copy link
Contributor Author

Ok, i have fixed stuff up a bit, hope you like it! :)

Copy link

@K009 K009 left a comment

Choose a reason for hiding this comment

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

Thanks for the further changes! Three general notes before digging into the code.

  1. Do you have an option to mark the conversation resolved? If yes please mark it that way. It would make the pull request more readable for us. I'm talking about such button:
    image

  2. It's always a good idea to make the whole demo responsive, so please don't remove the lines in your code that are resizing elements depending on the screen size. Our goal is to always make the demo work for all screen sizes. 😄

  3. I also suggest declaring such variable as const chart = this inside the render function, to make the whole function easier to read. Sometimes we might not be sure what this is referring to.

Copy link

@K009 K009 left a comment

Choose a reason for hiding this comment

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

Almost at the finish line, only one thing to fix! Besides that, I like the way of using xAxis to draw the categories in the center and other improvements.

@MarkusBarstad
Copy link
Contributor Author

Seems better now! removed that line and did just like you said, modified the tickPositions for that axis and set endOnTick to false ;)

Copy link

@K009 K009 left a comment

Choose a reason for hiding this comment

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

Great job, thanks for all the changes!

@MarkusBarstad
Copy link
Contributor Author

Thank you, man! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants