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

RichText & Docs: remove or rename undocumented functions and constants #14239

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 5, 2019

Description

This change removes all rich-text exceptions in the doc generation script.

I'm removing some functions that were never documented and are only used in a few cases. Elsewhere the properties are just accessed directly. insertLineBreak was added in #13546, but the complexity around it was removed in #13850 and since then it's also no longer useful. It was also never documented.

I'm also prefixing all the remaining exports that are undocumented with __unstable.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

aduth
aduth previously requested changes Mar 5, 2019
packages/rich-text/src/get-selection-start.js Show resolved Hide resolved
@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from 0793065 to cb2708b Compare March 5, 2019 14:01
@ellatrix ellatrix requested a review from aduth March 5, 2019 14:02
@aduth
Copy link
Member

aduth commented Mar 18, 2019

I don't really have any objections here, though it will need a rebase. Does the fact it missed the 5.3 release (WordPress 5.2) have any implications on your desire to move forward here?

@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from cb2708b to 5b2bef0 Compare March 28, 2019 08:36
@ellatrix ellatrix changed the title RichText: remove some undocumented functions RichText & Docs: remove or rename undocumented functions and constants Mar 28, 2019
@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from 5b2bef0 to d8dfa86 Compare March 28, 2019 09:18
@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from d8dfa86 to 1276de5 Compare March 28, 2019 09:20
@ellatrix ellatrix requested review from oandregal and aduth March 28, 2019 09:26
@ellatrix ellatrix added [Type] Developer Documentation Documentation for developers [Package] Rich text /packages/rich-text [Tool] Docgen /packages/docgen labels Mar 28, 2019
@ellatrix
Copy link
Member Author

I don't really have any objections here, though it will need a rebase. Does the fact it missed the 5.3 release (WordPress 5.2) have any implications on your desire to move forward here?

No, I'd still like to move forward with this. The same issue remains.

@ellatrix ellatrix added the [Type] Code Quality Issues or PRs that relate to code quality label Mar 28, 2019
@ellatrix ellatrix added this to the 5.4 (Gutenberg) milestone Mar 28, 2019
@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from 1276de5 to e3309c7 Compare March 28, 2019 10:38
export { getTextContent } from './get-text-content';
export { isCollapsed } from './is-collapsed';
export { isEmpty, isEmptyLine } from './is-empty';
export { isEmpty, isEmptyLine as __unstableIsEmptyLine } from './is-empty';
Copy link
Member

Choose a reason for hiding this comment

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

Renaming the symbol at this point to use the __unstable prefix will work as far as the doc generator is concerned because it uses the export statements to filter out the symbols to be ignored. However, I would lean towards renaming the symbol declaration as well. It may be benefitial as an urge to settle this API every time we work on it. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, maybe it's good to do that. But the reason it's marked unstable is that it was never meant to be exported. It's only used by the RichText component, which should eventually live in these packages. So it's meant as an internal helper function, temporarily exported because these's one component left that's not been moved to this package yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be benefitial as an urge to settle this API every time we work on it.

So, regarding this, there's nothing to be settled. We need to move RichText so we can remove the export statements.

@@ -35,31 +35,15 @@ const packages = [
'wordcount',
];

const getArgsForPackage = ( packageName ) => {
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@oandregal
Copy link
Member

I've tested this a bit based on the changes I saw. This is what I've found:

  • Hit INTRO in a paragraph splits it into two paragraph blocks but the first one becomes invalid.
  • Left/Right arrow keys don't work.

@ellatrix
Copy link
Member Author

ellatrix commented Apr 1, 2019

@nosolosw Oops, forgot to change an import statement after the rename. Fixed in 0b1182b. Thanks for testing!

@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from 0b1182b to db15896 Compare April 2, 2019 12:27
@aduth
Copy link
Member

aduth commented Apr 3, 2019

(Noting there are some merge conflicts here)

@ellatrix ellatrix force-pushed the remove/undocumented-rich-text-functions branch from db15896 to 7ca2c60 Compare April 3, 2019 15:48
@oandregal oandregal self-requested a review April 10, 2019 08:12
Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

Code changes make sense. Tested and couldn't find any breakage.

@oandregal
Copy link
Member

I did find an issue that I can repro on master, though: #14896

@ellatrix
Copy link
Member Author

Thanks @nosolosw!

@ellatrix ellatrix merged commit e6e5d85 into master Apr 10, 2019
@ellatrix ellatrix deleted the remove/undocumented-rich-text-functions branch April 10, 2019 09:24
mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
WordPress#14239)

* RichText & Docs: remove or rename undocumented functions and constants

* Fix import after renaming
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Rich text /packages/rich-text [Tool] Docgen /packages/docgen [Type] Code Quality Issues or PRs that relate to code quality [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants