-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat(zoom component): added Zoom Component and olmincss from olv6.4 #47
base: main
Are you sure you want to change the base?
Conversation
Added +,- zoom component in map:Controls re orcasound#46
Deploy request for orcamap accepted. Accepted with commit 8ae7ec4 https://app.netlify.com/sites/orcamap/deploys/6069ac2934f6a60007ab8d59 |
@@ -0,0 +1,267 @@ | |||
.ol-box { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanoats added this files to include css styles necessary with openlayers, we can also define our own components, but this is standard stylesheet provided by https://cdn.jsdelivr.net/gh/openlayers/openlayers.github.io@master/en/v6.5.0/css/ol.css
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanoats I think that adding these options in the state machinery as initiated by @DhananjayPurohit #5 here will help us plan better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivanoats @parthsarthiprasad i also think olmincss files are not needed because we would be working with our own set of components and this would only add extra load to file structure
Hey @Devesh21700Kumar maybe I can add Seperate component wise CSS but if we are using ol provided components, class name will inevitably be same, since it's 260 line file so minified form wouldn't hurt much but I agree I can remove few unnecessary definitions for now. ( which we might add later). |
So, It's a PR based on UI for zoom component. I think keeping a state of zoom level too can help us to shelter the features like double-tapping a hydrophone can adjust the zoom to a definite level so an end-user can focus only on that specific hydrophone. @parthsarthiprasad @Devesh21700Kumar I am curious to know where you can place that in the state machine (#5 ). |
I like the provision of feature of double tapping and keeping zoom state value, if it's provided in future @DhananjayPurohit Currently this follows standards like fullscreen control being used already and would love any suggestions for sure. |
Added +,- zoom component in map:Controls
re #46
Pull request checklist
Please check if your PR 🚀 fulfills the following requirements:
npm run start
) was run locally and new changes were working correctlynpm run linter
) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Fixes #
Fixed feature request for #46
Old Component:
New Component
What is the new behavior?
Does this introduce a breaking change?
Checklist_User's_Behalf:
Other information