-
Notifications
You must be signed in to change notification settings - Fork 4k
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
perf(docs): optimize ComponentProps #2012
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2012 +/- ##
=======================================
Coverage 99.76% 99.76%
=======================================
Files 148 148
Lines 2561 2561
=======================================
Hits 2555 2555
Misses 6 6 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levithomason I wanted to add there changes that will affect ComponentExample
, but later I understood that it has already enough changes. So, it's ready for review.
docs/app/HOC/pure.js
Outdated
} | ||
} | ||
|
||
export default pure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this can be replaced by extending React.PureComponent
, correct?
{...item} | ||
key={item.name} | ||
showEnums={showEnumsFor[item.name]} | ||
onToggleEnum={this.toggleEnumsFor(item.name)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the showEnums
state is only needed within ComponentPropsRow
, I think we can push it down another level. The parent doesn't need to re-render when the child shows/hides enums.
@@ -7,7 +7,9 @@ import ComponentPropsExtra from './ComponentPropsExtra' | |||
import ComponentPropsToggle from './ComponentPropsEnumToggle' | |||
import ComponentPropsValue from './ComponentPropsEnumValue' | |||
|
|||
const ComponentPropsEnum = ({ limit, showAll, toggle, values }) => { | |||
const ComponentPropsEnum = ({ limit, showAll, toggle, type, values }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The limit
was added because of the Icon
names and Flag
names. However, those props do not show enums as they are {custom}
. I almost want to just show all enums and do away with the state.
At the least, let's increase the limit to 50
. Most examples have much less than 50 enums, and 50 will still fit OK. Here are a few examples of our larger lists that show we can easily work with a limit of 50:
Grid.Row
color
is especially funny as it a very common prop and we hide only 3 of them 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminding, customPropTypes.suggest
should be also parsed as enum
, I will update it separate PR.
…React into perf/docs-stage2 # Conflicts: # docs/app/Components/ComponentDoc/ComponentExample/ComponentExample.js
…toggleEnums logic down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@levithomason I've applied requested changes 👍
💥 |
Released in |
* perf(docs): optimize ComponentProps * fix(HOC): use PureComponent for pure() HOC * fix(ComponentProps): update `limit` value * feat(ComponentProps): make ComponentProps functional component, move toggleEnums logic down
ComponentProps
Components was splitted to multiple sub-components. Props info transforms was moved to our gulp doc plugin.
Before
After