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

feat(zoom component): added Zoom Component and olmincss from olv6.4 #47

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

parthsarthiprasad
Copy link
Contributor

@parthsarthiprasad parthsarthiprasad commented Apr 4, 2021

Added +,- zoom component in map:Controls

re #46

Pull request checklist

Please check if your PR 🚀 fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run start) was run locally and new changes were working correctly
  • Lint (npm run linter) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Fixes #

Fixed feature request for #46

Old Component:

image

New Component

image

What is the new behavior?

  • added zoomcontrol for map, limits max and minzoom
  • specifies projection standard for map
  • ( comments added to add minresolution, maxresolution in future )

Does this introduce a breaking change?

  • Yes
  • No

Checklist_User's_Behalf:

  • Documentation
  • Tests
  • TypeScript Types
  • Ready to be merged

Other information

@netlify
Copy link

netlify bot commented Apr 4, 2021

Deploy request for orcamap accepted.

Accepted with commit 8ae7ec4

https://app.netlify.com/sites/orcamap/deploys/6069ac2934f6a60007ab8d59

@@ -0,0 +1,267 @@
.ol-box {
Copy link
Contributor Author

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

Copy link
Contributor

@Devesh21700Kumar Devesh21700Kumar left a 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.

Copy link
Contributor

@Devesh21700Kumar Devesh21700Kumar left a 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

@parthsarthiprasad
Copy link
Contributor Author

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).

@DhananjayPurohit
Copy link
Member

DhananjayPurohit commented Apr 4, 2021

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 ).

@parthsarthiprasad
Copy link
Contributor Author

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.

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.

3 participants