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

Adds an additional zoom visibility property to markers to show/hide b… #208

Merged
merged 4 commits into from
Mar 31, 2020

Conversation

digitaldan
Copy link
Contributor

…ased on current zoom level.

Signed-off-by: digitaldan [email protected]

…ased on current zoom level.

Signed-off-by: digitaldan <[email protected]>
@digitaldan digitaldan requested a review from a team as a code owner March 29, 2020 20:24
@digitaldan
Copy link
Contributor Author

zoomVisibility

@digitaldan
Copy link
Contributor Author

digitaldan commented Mar 29, 2020

Hi @ghys was playing around with a way to be able to zoom into a map and display more markers based on the zoom level. Not sure if this is the best way to handle that. or if i even did it right :-)

@ghys
Copy link
Member

ghys commented Mar 30, 2020

Thanks @digitaldan, that's a nice feature! My only (small) concern is that users would need to figure out by themselves what the zoom level is, since it's neither shown anywhere nor retrieved easily, so it's not an option for everyone. Therefore I'd like to suggest making it an "advanced" option, meaning it will only be shown after checking a "show advanced options" box. Doing so is easy, the widgets have the same config parameter description format as other parts in openHAB so you just have to add advanced: true in your description, the checkbox will appear automatically.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

LGTM - just a few housekeeping remarks.

label: 'Zoom Visibility',
type: 'NUMBER',
description: 'Visible only when zoomed to at least this level (empty by default)'
},
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the trailing comma here, and add advanced: true, above (3rd line or so) as discussed above.

@@ -170,6 +170,14 @@ export default {
zoomControl:false
} : {})
},
markers(){
Copy link
Member

Choose a reason for hiding this comment

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

Please add spaces before and after the braces.

@ghys
Copy link
Member

ghys commented Mar 30, 2020

@digitaldan actually there quite a few linting errors in this and your previous PR (I overlooked it last time) 😅

image

I have a laxer approach than most of the other openHAB projects when it comes to enforcing them, in that they would not fail the build, but still it would be great if could sort them out while you're at it 😄.

If you're using VS Code as your IDE, searching for ESLint in the extension pane and installing dbaeumer.vscode-eslint should do the trick. When developing Vue files you'll want to install Vetur (octref.vetur) as well.

Then it should only be a matter of looking for the little lightbulb icons when hovering over code or in the problems window, and selecting 'Fix all auto-fixable problems':

image

Let me know how this works for you so we can eventually add this to the CONTRIBUTING file.

@digitaldan
Copy link
Contributor Author

My only (small) concern is that users would need to figure out by themselves what the zoom level is

I was thinking the same thing. I had played with displaying the zoom level when editing, but i think making it an advanced property makes sense. I may still play with displaying something.

I have a laxer approach than most of the other openHAB projects when it comes to enforcing them

I would not mind make making this stricter actually. There is a lot of code, and it really helps to have consistency. I don't mind making sure the code is properly linted before checking in..

extension pane and installing dbaeumer.vscode-eslint

Nice, that worked perfectly. I will start paying attention to the "problems" tab now :-)

@digitaldan
Copy link
Contributor Author

zoomVisibility

I added a zoom display when editing, thoughts?

@ghys
Copy link
Member

ghys commented Mar 30, 2020

I added a zoom display when editing, thoughts?

I like it! ;)

I would not mind make making this stricter actually. There is a lot of code, and it really helps to have consistency. I don't mind making sure the code is properly linted before checking in..

Yes, that could be valuable, you could go further and add the vue/recommended rules for stricter rules (right now there's only vue/essential and @vue/standard in .eslintrc), also why not trying out Prettier, which is widely praised, as well and checking a default VS Code settings file to fix problems on save.
Some details here: https://medium.com/@gogl.alex/how-to-properly-set-up-eslint-with-prettier-for-vue-or-nuxt-in-vscode-e42532099a9c.
For now I was happy with only having to fix basic problems highlighted in the editor but if there are more contributions over time we should probably have a plan to keep the code clean. I just don't want to deal with the backlog right now 😄

@digitaldan
Copy link
Contributor Author

FYI, there was an issue with markers flickering when zooming, my last push fixes this, but may seem counterintuitive at first glance

@digitaldan
Copy link
Contributor Author

Ignore my last comment and commit, there was something else goofy going on with my browser.

@digitaldan digitaldan changed the title [WIP] Adds an additional zoom visibility property to markers to show/hide b… Adds an additional zoom visibility property to markers to show/hide b… Mar 30, 2020
Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

@ghys ghys merged commit 723e55c into openhab:master Mar 31, 2020
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