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

Added configurability to Selection and Results #350

Merged
merged 3 commits into from
Feb 5, 2019

Conversation

theduckylittle
Copy link
Member

  • Added the code to allow configuring them from the application
    constructor.
  • Moved the style defintions to a defaults module for easier
    reference, maintenance, and testing down the road.
  • Added a doc to refer people to.

 * Added the code to allow configuring them from the application
   constructor.
 * Moved the style defintions to a defaults module for easier
   reference, maintenance, and testing down the road.
 * Added a doc to refer people to.

## Change the highlight color

Pass `resultsStyle.highlight` into the `loadMapbook` options.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't match the example code.

Copy link
Member

Choose a reason for hiding this comment

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

gm3.Application != loadMapbook

@@ -248,15 +229,15 @@ class Application {
url: options.url,
type: 'xml',
success: (response) => {
this.populateMapbook(response);
this.populateMapbook(response, options);
Copy link
Member

Choose a reason for hiding this comment

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

populateMapbook only takes one argument

}
});

} else if(options.content) {
// this is just straight mapbook xml
// TODO: Maybe it needs parsed?!?
const populate_promise = new Promise((resolve, reject) => {
this.populateMapbook(options.content);
this.populateMapbook(options.content, options);
Copy link
Member

Choose a reason for hiding this comment

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

populateMapbook only takes one argument

## GeoMoose vector styles

All vector styles are styled using the Mapbox GL Styles format. It is Javascript/JSON
friendly and well documented here. TODO: INSERT LINK.
Copy link
Member

Choose a reason for hiding this comment

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

TODO

Copy link
Member

Choose a reason for hiding this comment

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

Wow, that's a hard link to find. MapBox GL Styles are well documented, but I gather we must be using some subset of MapBox GL Styles since overall it looks like it more or less is a JSON version of a GeoMoose 2 mapbook.

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.mapbox.com/mapbox-gl-js/style-spec/#layers

If I had to guess it would be some combination of 'paint' and 'layout' properties (although I don't think we are splitting them out into separate categories).

Copy link
Member Author

Choose a reason for hiding this comment

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

There is some current merging of properties -- which is actually why upgrading the ol-mapbox-style library will require more of an upgrade than bumping the version number. The new versions are 'cleaner' about it and we're exercising the library in a way for which the spec was not originally intended.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. The other thing is if updating it breaks the current styles, it would force us into major release which is somewhat undesirable.

@klassenjs
Copy link
Member

Did we intend to change the colors in the demo (app.js)? I'd be tempted to leave the lines in app.js but comment them out. Otherwise, it looks ok to me.

@theduckylittle
Copy link
Member Author

Agreed, backed out the changes altogether.

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.

None yet

2 participants