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

updates #460

Merged
merged 5 commits into from
Sep 9, 2015
Merged

updates #460

merged 5 commits into from
Sep 9, 2015

Conversation

btfou
Copy link
Contributor

@btfou btfou commented Sep 8, 2015

Just a few housekeeping items; and fixes #457.

config = config || {};
var ip = new ImageParameters();
//image parameters for dynamic services, set to png32 for higher quality exports
ip.format = 'png32';
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be passed in as a parameter to the function for each layer instead of being hard-coded. Or at least mixed in so it can be overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just defaults to 'png32'. If config has {format: 'png8'} it will get overridden.

Copy link
Member

Choose a reason for hiding this comment

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

indeed it is and would. sorry.

@jebu75
Copy link
Collaborator

jebu75 commented Sep 9, 2015

I think this is good. I never liked having so much code baked into the html template. I don't think this would cause any backward compatibility issues so this could be released ahead of an eventual 2.0 release.

For my own apps, I moved the buildImageParameters function into a utility module so it could be used elsewhere but I think including it in viewer.js is a good example for those less familiar with CMV. We may want to make a note to re-visit this function with the 4.0 API. I believe in 4.0 all properties can be set as constructor arguments which could make this function a bit simpler, maybe even negate the need for it.

I think we should go ahead with these changes.

@tmcgee tmcgee added this to the v1.3.4 milestone Sep 9, 2015
tmcgee added a commit that referenced this pull request Sep 9, 2015
@tmcgee tmcgee merged commit e48424a into develop Sep 9, 2015
@tmcgee tmcgee deleted the btfou-updates-1 branch September 9, 2015 02:19
@tmcgee tmcgee modified the milestones: v2.0.0-beta.1, v1.3.4 Mar 3, 2017
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