[Docs] Fix some typos #256
Conversation
|
Do you really need a Contributor License Agreement for such trival non code fixes? |
|
Thank you so much for tracking all those mistakes ❤️❤️❤️
Frankly speaking, I don't know. But then, I'd go with the safe: yes. It just saves us from thinking about possible problems that could arise in all possible universes in the future. |
|
Hi @denisname! We received your CLA (along with 2 other PRs :)). I understand now, though, why you asked whether you have to sign it – if I had to do that via traditional mail, I'd be a bit surprised too (you could have signed it online). We'd like to thank you for your troubles with a small souvenir – could email me ([email protected]) your t-shirt size and address? |
| @@ -20,8 +20,8 @@ import { isString } from 'lodash-es'; | |||
| * | |||
| * @param {Document} doc Document used to create element. | |||
| * @param {String} name Name of the element. | |||
| * @param {Object} attributes Object keys will become attributes keys and object values will became attributes values. | |||
| * @param {Node|String|Array.<Node|String>} children Child or array of children. Strings will be automatically turned | |||
| * @param {Object|null} [attributes] Object keys will become attributes keys and object values will became attributes values. | |||
There was a problem hiding this comment.
The null is unnecessary, since the param is marked as optional.
| @@ -12,7 +12,7 @@ import global from './global'; | |||
| /** | |||
| * For a given element, returns the nearest ancestor element which CSS position is not "static". | |||
| * | |||
| * @param {HTMLElement} element Native DOM element to be checked. | |||
| * @param {HTMLElement} [element] Native DOM element to be checked. | |||
There was a problem hiding this comment.
What would this function be doing if the element was not specified? This param is required for it do work.
There was a problem hiding this comment.
It returns null. There is a test for it. That said, required make more sense indeed.
| @@ -43,7 +43,7 @@ export const keyCodes = generateKnownKeyCodes(); | |||
| * | |||
| * Note: Key names are matched with {@link module:utils/keyboard~keyCodes} in a case-insensitive way. | |||
| * | |||
| * @param {String|module:utils/keyboard~KeystrokeInfo} Key name (see {@link module:utils/keyboard~keyCodes}) | |||
| * @param {String|module:utils/keyboard~KeystrokeInfo} key name (see {@link module:utils/keyboard~keyCodes}) | |||
There was a problem hiding this comment.
That's a sentence so it should start with an uppercased letter.
There was a problem hiding this comment.
When looking in documentation (https://ckeditor.com/docs/ckeditor5/latest/api/module_utils_keyboard.html#static-function-getCode), there is
getCode( Key ) → Number
Although all functions always have they parameters with lower case. Also in the Parameters part, sentence is broken between "key" and "name".
What about the following code instead (twice key)?
* @param {String|module:utils/keyboard~KeystrokeInfo} key Key name (see {@link module:utils/keyboard~keyCodes})
By the way, how do you generate the documentation? I did not find anything in the doc about it. Do you use internal tools?
There was a problem hiding this comment.
Hi, @denisname!
By the way, how do you generate the documentation? I did not find anything in the doc about it. Do you use internal tools?
Yes, we use the JSDoc3 package with a few custom plugins available here - https://github.com/ckeditor/ckeditor5-dev/tree/master/packages/jsdoc-plugins to generate proper AST tree and then we generate the documentation using our internal tool called Umberto.
What about the following code instead (twice key)?
* @param {String|module:utils/keyboard~KeystrokeInfo} key Key name (see {@link module:utils/keyboard~keyCodes})
Yep, you're right, the sentence should include both keys as the first key must match the param's name.
Btw I've proposed ckeditor/ckeditor5#1415, it should solve many of such typos and small documentation errors.
| @@ -81,7 +81,7 @@ export default class KeystrokeHandler { | |||
| * @param {String|Array.<String|Number>} keystroke Keystroke defined in a format accepted by | |||
| * the {@link module:utils/keyboard~parseKeystroke} function. | |||
| * @param {Function} callback A function called with the | |||
| * {@link module:engine/view/observer/keyobserver~KeyEventData key event data} object and | |||
| * {@link module:utils/keyboard~KeystrokeInfo keystroke info} object and | |||
There was a problem hiding this comment.
That's a bit messy now, but it's actually called with KeyEventData. The KeyEventData objects implements KeystrokeInfo but it adds preventDefault() and stopPropagation() methods (which are mentioned later in this sentence. It'd be good to not rely on engine's object type, so that's why I said it's messy. But other than that, KeyEventData is more correct.
|
I merged the PR. Some changes weren't correct (I left comments), but most of them were. Thank you! |
Examples for fixes can be found in
tests/.There are some typos I don't know how to fix:
altKey,ctrlKeyandshiftKeyare all optional.fitInViewportandlimiterare all optional.eventsis...String.bindPropertiesis...String.unbindPropertiesis...String.