Optimize frontend components #263
Conversation
sgomes
left a comment
sgomes
left a comment
There was a problem hiding this comment.
LGTM! 👍
I left a few comments, and I think this is probably best reviewed by someone that has a deeper understanding of the codebase as well, but the approach and the code look good.
Thank you again for working on this, @CGastrell! 🙏
| // input, | ||
| // textarea { | ||
| // box-sizing: border-box; | ||
| // font-size: var(--crowdsignal-forms-text-size) !important; | ||
| // padding: 16px !important; | ||
| // } |
There was a problem hiding this comment.
You may have missed this when creating the PR. Probably something you intended to delete?
There was a problem hiding this comment.
Cleaned up, thanks!
client/blocks/feedback/edit.js
Outdated
| <Tooltip | ||
| text={ promoteLink } | ||
| position="top center" | ||
| > | ||
| <a | ||
| href="https://crowdsignal.com/pricing" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="crowdsignal-forms__branding-promote" | ||
| > | ||
| { __( | ||
| 'Hide', | ||
| 'crowdsignal-forms' | ||
| ) } | ||
| </a> | ||
| </Tooltip> |
There was a problem hiding this comment.
You may have considered this already, but one possibility would be to extract this to a separate module, to be used for both feedback/edit.js and nps/edit.js. That would still keep it out of the frontend, and should be handled just fine by the build process.
There was a problem hiding this comment.
Yeah, I think I was mostly dashing and slashing until I got it out, likely suspect to get into a module on its own.
There was a problem hiding this comment.
I moved the tooltip to a separate component so to reuse it. Thanks for the suggestion!
client/components/nps/index.js
Outdated
| onClick={ onClose } | ||
| > | ||
| <Icon icon="no-alt" /> | ||
| X |
There was a problem hiding this comment.
Suggest using ✖ or ✕ instead, which are a bit more visually distinct from the letter X.
Alternatively, you could embed a tiny SVG here; in general it's a bad practice to do SVG-in-JS, but if it's something tiny that you're using in a single spot, that's fine.
| import { Component } from '@wordpress/element'; | ||
| import { createHigherOrderComponent } from '@wordpress/compose'; | ||
|
|
||
| export const withWordPressFallbackStyles = ( mapNodeToProps ) => |
There was a problem hiding this comment.
Good call on moving this method here! 👍
Given the way that Gutenberg recommends externs for WP and how big @wordpress/components is, it's really not a great idea to have utility methods in there 😕
…the frontend components
CGastrell
force-pushed
the
remove/wp-components-dependency
branch
from
7936f3a to
29833b0
Compare
This PR attempts to remove, as far as possible, dependencies on @wordpress/components on the frontend components to lower the size of the builds and improve render performance.