-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add create-image script to generate example thumbnails #8029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works really well. The script doesn't recreate the current images as seen here:
But I think that's fine since that's not what we're trying to do. I guess the difference is that the current photos weren't captured at 1200x500 pixels but were cut down from larger images, from what I understand.
I made a couple of suggestions but this is 👍
docs/bin/create-image.js
Outdated
const pack = require('../../package.json'); // eslint-disable-line | ||
|
||
const fileName = process.argv[2]; | ||
const token = process.argv[3] || process.env.MapboxAccessToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extend this to also check for process.env.MAPBOX_ACCESS_TOKEN
? that variable is included in some other places like the benchmark tests and I know personally that I have my token saved locally under that name. I would assume others may as well.
docs/bin/create-image.js
Outdated
const fileNameFormatted = fileName.replace('.html', '').replace('.js', ''); | ||
// get the example contents/snippet | ||
const snippet = require('fs').readFileSync(path.resolve(__dirname, '..', 'pages', 'example', `${fileNameFormatted}.html`), 'utf-8'); | ||
// create an HTML page to display to example snippet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should probably read create an HTML page to display the example snippet
. to
-> the
package.json
Outdated
@@ -36,6 +36,7 @@ | |||
"murmurhash-js": "^1.0.0", | |||
"pbf": "^3.0.5", | |||
"potpack": "^1.0.1", | |||
"puppeteer": "^1.13.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only intended to be run manually by someone adding a new example, then I think we should move puppeteer
to devDependencies
so it's not installed as part of the main bundle.
Thanks for the feedback @ryanhamley! I've incorporated your suggestions.
That's a good point! The original screenshots were also retina, so I played around with puppeteer's viewport and was able to force a very similar scale. I think that should keep us consistent. |
docs/bin/create-image.js
Outdated
const pack = require('../../package.json'); // eslint-disable-line | ||
|
||
const fileName = process.argv[2]; | ||
const token = process.argv[3] || process.env.MAPBOX_ACCESS_TOKEN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I meant that it would be good to check for MAPBOX_ACCESS_TOKEN
in addition to MapboxAccessToken
since people may have it saved either way. That's what the benchmark tests do as seen here.
docs/README.md
Outdated
- png format | ||
2. Run `npm run build-images` to generate the appropriate images. Alternatively, when you run `npm run start-docs` the site will generate the images. | ||
3. Run the site locally to verify that your example image is loading as expected. | ||
1. Run `npm run create-image <example-file-name> <mapbox-access-token>`. The script will take a screenshot of map in the example and save it to `docs/img/src/`. Commit the image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should read The script will take a screenshot of the map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I'd suggest checking for either naming convention on the access token but this looks great.
Just a note that |
* publisher-production: (41 commits) bump publisher after timeout (#8058) cosmetic update to draggable Marker/point examples to prevent coordinates overlapping Mapbox logo (#8057) update mapbox style and tileset version numbers to most recent (#8056) update docs page shell (#8039) [email protected] Add create-image script to generate example thumbnails (#8029) Add HTML clusters + aggregated properties example (#8019) add 3D model with three.js example (#7977) update mapbox-gl-geocoder to @3.1.4 in examples (#7978) V0.53.1 (#7961) rename example for accepting coordinates in the geocoder (#7859) [docs] use appropriate-images for examples to increase page speed (#7900) [docs] dr-ui 0.6.0 (#7909) version bump gl-geocoder to v3.1.2 (#7924) Fix docs for Camera methods Docs page shell update 02-14-19 (#7907) fixes regression with css on examples pages (#7891) updates sentry project for docs subdomain (#7890) v0.53.0 (#7884) [docs] update dr-ui 0.5.0 (#7880) ...
* publisher-production: (41 commits) bump publisher after timeout (#8058) cosmetic update to draggable Marker/point examples to prevent coordinates overlapping Mapbox logo (#8057) update mapbox style and tileset version numbers to most recent (#8056) update docs page shell (#8039) [email protected] Add create-image script to generate example thumbnails (#8029) Add HTML clusters + aggregated properties example (#8019) add 3D model with three.js example (#7977) update mapbox-gl-geocoder to @3.1.4 in examples (#7978) V0.53.1 (#7961) rename example for accepting coordinates in the geocoder (#7859) [docs] use appropriate-images for examples to increase page speed (#7900) [docs] dr-ui 0.6.0 (#7909) version bump gl-geocoder to v3.1.2 (#7924) Fix docs for Camera methods Docs page shell update 02-14-19 (#7907) fixes regression with css on examples pages (#7891) updates sentry project for docs subdomain (#7890) v0.53.0 (#7884) [docs] update dr-ui 0.5.0 (#7880) ...
refs #8025
To make it easier for contributors who are adding new examples, I created
npm run create-image
that will take a screenshot of the new example's map and save it to the correct directory. The script also saves the image using our specifications of size and format.For the script to run, the contributor will need to pass an Mapbox access token to the command so that the map can load for the screenshot. While this adds a small amount of friction, I think it still streamlines the process of generating these thumbnail images. I'd be happy to hear any suggestions on implementation.