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 documentation to the Editable element #3346

Merged
merged 5 commits into from Nov 27, 2017

Conversation

Projects
None yet
4 participants
@atimmer
Member

atimmer commented Nov 6, 2017

Description

I've added documentation to the Editable element class. I have stopped midway because it takes an incredible amount of time and I want to focus on other Gutenberg related work. This also highlights why it is so incredibly important that documentation be written by the original code author. I don't have the necessary context to write this stuff.

How Has This Been Tested?

It is only documentation

Screenshots (jpeg or gifs if applicable):

Types of changes

  • Bug fix (non-breaking change which fixes an issue), we should consider missing documentation a bug.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 6, 2017

Codecov Report

Merging #3346 into master will increase coverage by 2.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3346      +/-   ##
==========================================
+ Coverage   34.54%   36.81%   +2.26%     
==========================================
  Files         261      271      +10     
  Lines        6710     6647      -63     
  Branches     1225     1202      -23     
==========================================
+ Hits         2318     2447     +129     
+ Misses       3704     3547     -157     
+ Partials      688      653      -35
Impacted Files Coverage Δ
blocks/editable/index.js 10.23% <ø> (ø) ⬆️
components/popover/index.js 80.5% <0%> (-4.26%) ⬇️
blocks/api/validation.js 91.46% <0%> (-3.7%) ⬇️
editor/components/inserter/menu.js 85.54% <0%> (-1.79%) ⬇️
blocks/api/factory.js 86.48% <0%> (-0.7%) ⬇️
editor/reducer.js 90% <0%> (-0.28%) ⬇️
editor/selectors.js 92.85% <0%> (-0.27%) ⬇️
blocks/api/serializer.js 98% <0%> (-0.19%) ⬇️
blocks/api/parser.js 98% <0%> (-0.08%) ⬇️
blocks/hooks/anchor.js 80% <0%> (ø) ⬆️
... and 141 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38d0c74...3c7205e. Read the comment docs.

codecov bot commented Nov 6, 2017

Codecov Report

Merging #3346 into master will increase coverage by 2.26%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3346      +/-   ##
==========================================
+ Coverage   34.54%   36.81%   +2.26%     
==========================================
  Files         261      271      +10     
  Lines        6710     6647      -63     
  Branches     1225     1202      -23     
==========================================
+ Hits         2318     2447     +129     
+ Misses       3704     3547     -157     
+ Partials      688      653      -35
Impacted Files Coverage Δ
blocks/editable/index.js 10.23% <ø> (ø) ⬆️
components/popover/index.js 80.5% <0%> (-4.26%) ⬇️
blocks/api/validation.js 91.46% <0%> (-3.7%) ⬇️
editor/components/inserter/menu.js 85.54% <0%> (-1.79%) ⬇️
blocks/api/factory.js 86.48% <0%> (-0.7%) ⬇️
editor/reducer.js 90% <0%> (-0.28%) ⬇️
editor/selectors.js 92.85% <0%> (-0.27%) ⬇️
blocks/api/serializer.js 98% <0%> (-0.19%) ⬇️
blocks/api/parser.js 98% <0%> (-0.08%) ⬇️
blocks/hooks/anchor.js 80% <0%> (ø) ⬆️
... and 141 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38d0c74...3c7205e. Read the comment docs.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 6, 2017

Contributor

This also highlights why it is so incredibly important that documentation be written by the original code author. I don't have the necessary context to write this stuff.

This also highlights what I raised in the core-js meeting. It doesn't make sense to me to add documentation to React component methods like those. These are private functions and change constantly. The time spent to document them is a wasted time because we do not need this documentation to understand the code (for most of them). I propose to be selective and document only the "tricky" parts.

Contributor

youknowriad commented Nov 6, 2017

This also highlights why it is so incredibly important that documentation be written by the original code author. I don't have the necessary context to write this stuff.

This also highlights what I raised in the core-js meeting. It doesn't make sense to me to add documentation to React component methods like those. These are private functions and change constantly. The time spent to document them is a wasted time because we do not need this documentation to understand the code (for most of them). I propose to be selective and document only the "tricky" parts.

@tomjn

This comment has been minimized.

Show comment
Hide comment
@tomjn

tomjn Nov 6, 2017

Contributor

Docs should get written and rewritten as code changes, not as an after thought or something that happens at the end, these kinds of inline docs are good for people who want to contribute, and people who can contribute are in short supply

Contributor

tomjn commented Nov 6, 2017

Docs should get written and rewritten as code changes, not as an after thought or something that happens at the end, these kinds of inline docs are good for people who want to contribute, and people who can contribute are in short supply

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 7, 2017

Member

There are some lint errors in this branch for invalid JSDoc.

Member

aduth commented Nov 7, 2017

There are some lint errors in this branch for invalid JSDoc.

Show outdated Hide outdated blocks/editable/index.js Outdated

atimmer added some commits Nov 16, 2017

Show outdated Hide outdated blocks/editable/index.js Outdated
Show outdated Hide outdated blocks/editable/index.js Outdated
@aduth

aduth approved these changes Nov 27, 2017

Nice 👍

@aduth aduth merged commit d5711dc into WordPress:master Nov 27, 2017

2 checks passed

codecov/project 36.81% (+2.26%) compared to 38d0c74
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@atimmer atimmer deleted the atimmer:fix/document-editable-block branch Dec 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment