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

Canvas source initialization from HTML element #6424

Merged
merged 8 commits into from
Apr 6, 2018
Merged

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Mar 29, 2018

  • Modifies CanvasSource#canvas property to accept either a string denoting the ID of the canvas element from which to read (as it previously functioned) or an HTMLCanvasElement (fixes Canvas source initialization from HTML element, instead of element id #6372)
  • Introduces validation warnings — while I doubt we have many users initializing canvas sources from stylesheets rather than via map.addSource, it's possible (and in fact this was done by our own debug example). It seems abrupt to deprecate this quickly and without warning, so validation now warns and does not fail on canvas sources added in the stylesheet.

Closes #6372.
Supersedes #6403.
Ref #5545.

Docs now show a separate CanvasSourceOptions typedef (🙇‍♀️ thank you @jfirebaugh):
image

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page

@lbud
Copy link
Contributor Author

lbud commented Mar 29, 2018

Ah oops, not quite ready until I fix the Style Spec docs page — working on that. fixed

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Looks great! Two things I'm not sure of:

  • Should we continue to allow canvas sources in setStyle and the Map constructor? (With the validator skipping canvas sources rather than warning.) We like it when people use smart setStyle, so it seems unfortunate to prevent that in combination with a canvas source.
  • Canvas#serialize includes the canvas property in the result, which is the HTMLCanvasElement. It looks like this has always been the case, but is this an issue?

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

Looks great to me.

Regarding @jfirebaugh 's point about setStyle, I'm torn. I agree that we want to encourage the use of setStyle, but I also think there are advantages to keeping MapOptions#style / the setStyle argument spec-compliant. One of those advantages is clarity: it's simpler to describe what style is allowed to be if it's just "a style spec document", rather than "a style spec document, plus canvas sources, custom sources, and custom layers".

Alternative: deprecate canvas in style objects (as is currently happening in this PR), and plan a Map#setState() API that accepts a style, the desired camera state, and extra-style-spec items like canvas/custom sources, custom layers.

@lbud
Copy link
Contributor Author

lbud commented Apr 3, 2018

One of those advantages is clarity: it's simpler to describe what style is allowed to be if it's just "a style spec document", rather than "a style spec document, plus canvas sources, custom sources, and custom layers".

I agree the clarity would be nice, but I think since at the moment our current best/only recommendation for "how do I persist my overlays while changing the basemap" is to use setStyle well, we should at least for now allow canvas sources in setStyle without emitting warnings 🛠

@anandthakker
Copy link
Contributor

Yeah, I guess deprecating this without yet having an alternative doesn't really make sense

@lbud
Copy link
Contributor Author

lbud commented Apr 4, 2018

As of 79a84c2 we silence validation errors specifically for canvas sources. This makes it pretty easy to rip out in the future (in the event of a setState API as in @anandthakker's suggestion), while erroring if the validator is used by consumers other than mapbox-gl-js, i.e. the styles API. (This is a breaking change that could in theory hurt any users who have already uploaded stylesheets with canvas sources, but I can't imagine there are any… 🤔 )

@lbud lbud requested a review from anandthakker April 5, 2018 17:36
[-122.51423120498657, 37.56161849366671]
]
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can/should this be reverted now?

@lbud lbud changed the base branch from master to release-0.45 April 6, 2018 00:16
@lbud lbud merged commit ee81c7e into release-0.45 Apr 6, 2018
@lbud lbud deleted the canvas-el-2 branch April 6, 2018 00:28
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