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

Make x and y factor configurable #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

spapas
Copy link

@spapas spapas commented Jul 11, 2017

I believe that the icons are too big especially if you want to display many boats so the x_fac and y_fac are changed to options. This should also help with #1 since you can change the x and y fac depending on the zoom level (and your needs).

@thomasbrueggemann
Copy link
Owner

Thanks for your interest in the project and the pull request. I like the idea to change the x_fac/y_fac a lot! I noticed a couple of things:

  • It would be great to not expose the x_fac and y_fac variables as parameters, but rather one scale parameter that has a reasonable interval. Maybe from 0 to 100? And I would expose only one value to not confuse users

  • If I set a x_fax/y_fac of 0.8 the boat is rendered larger than the canvas element. I guess the canvas has go scale with the x_fac/y_fac values or a maximum and minimum value needs to be defined. That maybe goes along with the point above

  • The boat position on canvas and wind-indicator also should adjust to the different size

  • Updating the boatmarker.min.js file would be great

  • Updating the README would be great. It is missing some option parameters already, I know ;-)

* Add a single scale parameter (from 1 to 100)
* Update readme
* Update .min.js file
@spapas
Copy link
Author

spapas commented Aug 4, 2017

Hello @thomasbrueggemann, I've added a new changeset with most changes you recommend. Unfortunately, I'm not very familiar with canvas drawing so I didn't know how to change the boat position and wind-indicator per canvas as your request. The other recommendations have been implemented:

  • Added a scale parameter (instead of the x_fac and y_fac) with values 1 to 100 - this fixes the first and second point
  • Updated the boatmarket.min.js - this fixes the fourth point
  • Updated the README with a description of the scale option - fixes the fifth point

So only the third point remains but that's not something I can fix :/

@thomasbrueggemann
Copy link
Owner

Thanks a lot, @spapas .
I will look into it, maybe I find an easy way to adjust the boat position on the canvas ✌️

@spapas
Copy link
Author

spapas commented Aug 4, 2017

Cool thanks!

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