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.

Sign up for GitHub

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

Jump to bottom

Fix spelling #5349

Merged
merged 1 commit into from Dec 21, 2020
Merged

Fix spelling #5349

merged 1 commit into from Dec 21, 2020

Conversation

jbampton
Copy link
Contributor

Thanks for your interest in plotly.js!

Translations:

  • Please @ mention a few other speakers of this language who can help review your translations.
  • If you've omitted any keys from dist/translation_keys.txt - which means they will fall back on the US English text - just make a short comment about why in the PR description: the English text works fine in your language, or you would like someone else to help translating those, or whatever the reason.
  • You should only update files in lib/locales/, not those in dist/

Features, Bug fixes, and others:

Before opening a pull request, developer should:

  • git rebase their local branch off the latest master,
  • make sure to not git add the dist/ folder (the dist/ is updated only on version bumps),
  • make sure to commit changes to the package-lock.json file (if any new dependency required),
  • write an overview of what the PR attempts to do,
  • select the Allow edits from maintainers option (see this article for more details).

Note that it is forbidden to force push (i.e. git push -f) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please git merge master into your PR branch instead of git rebase master.

archmoj
archmoj reviewed Dec 17, 2020
View reviewed changes
Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

Looking good to me.
I am interested to know how you catch all these?
Please provide this info in the PR description instead of the template.
Thank you!

test/jasmine/tests/mock_test.js
figs['mapbox_density-mulitple'] = require('@mocks/mapbox_density-mulitple');
figs['mapbox_density-mulitple_legend'] = require('@mocks/mapbox_density-mulitple_legend');
figs['mapbox_density-multiple'] = require('@mocks/mapbox_density-multiple');
figs['mapbox_density-multiple_legend'] = require('@mocks/mapbox_density-multiple_legend');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please rename the mock and baseline files at following addresses?

test/image/mocks/mapbox_density-mulitple.json
test/image/mocks/mapbox_density-mulitple_legend.json
test/image/baselines/mapbox_density-mulitple_legend.png
test/image/baselines/mapbox_density-mulitple.png

test/jasmine/tests/range_slider_test.js
@@ -266,7 +266,7 @@ describe('Visible rangesliders', function() {
rangeslider: {visible: true}
},
width: 800,
hieght: 500
height: 500
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.
Let's remove this line, so that the default height is applied as before.

@archmoj
Copy link
Contributor

archmoj commented Dec 21, 2020

💃
Merging...
I'll edit the requested changes in the next PR.

@archmoj archmoj merged commit a5b295f into plotly:master Dec 21, 2020
@jbampton jbampton deleted the fix-spelling branch December 21, 2020 13:51
@jbampton
Copy link
Contributor Author

Hey @archmoj I can do a PR now and fix them if you want ?

@archmoj
Copy link
Contributor

archmoj commented Dec 21, 2020

Hey @archmoj I can do a PR now and fix them if you want ?

I am about to submit a PR. So no worries there.
But please just edit the PR description here and if you let us know how you catch these? #5349 (review)
I am curious to know if we could possibly add a test to avoid these mistakes.

@archmoj archmoj mentioned this pull request Dec 21, 2020
Merged
jbampton added a commit to jbampton/plotly.js that referenced this pull request Dec 21, 2020
@jbampton
feat(CI): add a GitHub Action to check spelling
1dba7ca 
Found a better way to check spelling.  This Go tool does find mistakes in source code as well as Markdown. From my experience it does not work on all file types or find all the errors, but it does seem to work well on code comments
- refs plotly#5349
- refs plotly/dash#1081
Using -> https://github.com/client9/misspell
@jbampton jbampton mentioned this pull request Dec 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archmoj archmoj archmoj left review comments

Assignees
No one assigned
Labels
status: reviewable type: community type: documentation type: maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jbampton @archmoj