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

Minor updates, typos, add vscode auto formatting #152

Merged
merged 2 commits into from
Aug 19, 2022

Conversation

jahow
Copy link
Contributor

@jahow jahow commented Aug 19, 2022

  • corrected several minor typos
  • reworded a few parts
  • fixed/added some code extracts
  • fixed params for getting ngrok to run
  • rewrote the heading part of the mobile app; it wasn't working on my mobile, now it works on chrome only; this might be worth checking some more
  • added a .vscode/settings file to enable auto formatting in vscode; I found that when copy pasting the resulting code wouldn't look great without this

Another small thing I've noticed during the workshop: npm install fails on vite with node 14.17

$ npm install
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'vite@3.0.8',
npm WARN EBADENGINE   required: { node: '^14.18.0 || >=16.0.0' },
npm WARN EBADENGINE   current: { node: 'v14.17.0', npm: '7.18.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'vite-plugin-static-copy@0.7.0',
npm WARN EBADENGINE   required: { node: '^14.18.0 || >=16.0.0' },
npm WARN EBADENGINE   current: { node: 'v14.17.0', npm: '7.18.1' }
npm WARN EBADENGINE }

added 241 packages, and audited 242 packages in 6s

66 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

$ npm -v
7.18.1

$ node -v
v14.17.0

I think it would make sense to advertise node 14.18 (or node 16) as a minimum requirement?

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

I think it makes sense to require at least Node 16, and I added a suggestion for the device orientation example.

src/en/examples/mobile/compass.js Outdated Show resolved Hide resolved
@jahow
Copy link
Contributor Author

jahow commented Aug 19, 2022

I'm not adding any node requirement in the package.json for now to avoid unnecessary failures on npm install, maybe after FOSS4G ;) thanks for the review!

@jahow jahow merged commit 16c6ab6 into openlayers:main Aug 19, 2022
@jahow jahow deleted the minor-updates branch August 19, 2022 12:40
@tschaub
Copy link
Member

tschaub commented Aug 19, 2022

I removed the src/en/.vscode/settings.json file in #153. I agree these are nice settings (and use them as well), but I think it works better if we do not commit OS- or editor-specific configuration files to the repo. Instead, I think it works well if you add your favorite editor's configuration files to your global gitignore file. Then we don't need to commit files for every OS/editor and we don't have to worry about conflicts in editor preferences.

So an example global ~/.gitignore file might look something like this:

.DS_Store
.vscode/

Then I don't need to worry about adding files matching those patterns to every repo, and I don't need to add them to every project's gitignore file.

Hope this doesn't sound pedantic. In this case, I already had a src/en/.vscode/settings.json file with settings that conflicted with the ones you added here (I have slightly different formatting settings at the user level, and I had a workspace setting here that override one of my user settings).

Thanks for the typo fixes and grammar changes.

I was able to get the heading stuff to work on Safari on my phone fwiw (with your changes).

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

3 participants