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

Revamp plugin documentation & strip plugin publishing functionality #4305

Merged
merged 5 commits into from
Feb 23, 2017

Conversation

lucaswoj
Copy link
Contributor

This PR closes #3947, closes #3687, and removes 74,231 lines from source control 😄

Definitely looking for feedback on the design of the plugins directory & taxonomy of plugins.

Any design suggestions @tristen @mayagao?

screen shot 2017-02-20 at 3 57 55 pm

Related to mapbox/mapbox-gl-directions#121

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.

👍 Simpler is better in this case.

Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

Nice! Just a couple outstanding qs.
Have you added publishing scripts to CI for all the Mapbox supported plugins?

tags:
- controls-and-overlays
---
<script src='https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-geocoder/v{{site.data.plugins.mapbox-gl-geocoder.latest}}/mapbox-gl-geocoder.js'></script>
<link rel='stylesheet' href='https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-geocoder/v{{site.data.plugins.mapbox-gl-geocoder.latest}}/mapbox-gl-geocoder.css' type='text/css' />
<script src='https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-geocoder/v2.0.1/mapbox-gl-geocoder.js'></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the plan for keeping these versions up to date?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan is for these to be manually updated alongside plugin releases, as they are now.

@lucaswoj
Copy link
Contributor Author

Have you added publishing scripts to CI for all the Mapbox supported plugins?

Not yet. Waiting for final 👍 on the mapbox-gl-directions PR before copying that code across all of our plugins.

jfirebaugh and others added 3 commits February 20, 2017 17:19
This version added read-only stub properties for clientWidth, offsetWidth, etc., so we need to use Object.defineProperty to override the new default value of 0.
@tristen
Copy link
Member

tristen commented Feb 21, 2017

LGTM! Tiny nit but I'd make a couple spacing changes to define these groups better:

screen shot 2017-02-21 at 10 34 18 am

@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 21, 2017

Todo Before Merging

@mcwhittemore
Copy link
Contributor

Maybe as a next step, it would be cool if the script that sends the distribution file to s3, also created a version file that could be used for auto updating the version numbers here. That said, this is a great leap forward towards making releases better. Thanks!

@lucaswoj
Copy link
Contributor Author

@mollymerp I finished adding deployment processes to all existing Mapbox plugins. Can you rereview? 🙏

@mollymerp
Copy link
Contributor

Nice work @lucaswoj! 💪 ✨

@lucaswoj lucaswoj merged commit ca48d1b into mb-pages Feb 23, 2017
@lucaswoj lucaswoj deleted the simplify-plugins branch February 23, 2017 00:34
@andrewharvey
Copy link
Collaborator

@lucaswoj With this merged, does it make sense to remove the list in the README.md and simply point to https://www.mapbox.com/mapbox-gl-js/plugins/ ? For example the mapbox-gl-directions plugin is missing in README.md so the risk of them becoming out of sync is real.

@lucaswoj
Copy link
Contributor Author

Yes @andrewharvey. That was my intention. Submitting a PR for the README.md in a moment.

@mollymerp mollymerp mentioned this pull request Jul 20, 2017
5 tasks
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.

6 participants