Merged
Conversation
added 9 commits
August 24, 2022 21:37
ajlende
added
[Type] Code Quality
Contributor
Author
|
In the last couple commits I refactored a few of the other types to follow TS best practices |
|
Size Change: -1.08 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Aug 25, 2022
|
|
||
| function createBorderGenerateFunction( path: string[] ): GenerateFunction { | ||
| return ( style, options ) => | ||
| generateRule( style, options, path, camelCase( path.join( ' ' ) ) ); |
Member
There was a problem hiding this comment.
I'd say we don't need an external library here.
There's already a util called upperFirst. See the unit test.
So, until we need extra functionality, maybe we could do the following:
const path = [ 'some', 'path', 'to', 'somewhere' ];
console.log( path.map( ( value, index) => index !== 0 ? upperFirst( value ) : value ).join( '' ) );
// -> somePathToSomewhereOr we could roll our own camelCase using upperFirst():
const path = [ 'some', 'path', 'to', 'somewhere' ];
function camelCase( [ firstItem, ...rest ] ) {
return firstItem.toLowerCase() + rest.map( upperFirst ).join( '' );
}
console.log( camelCase( path ) );
// -> somePathToSomewhereWhat do you reckon?
Contributor
Author
There was a problem hiding this comment.
I was using the lodash version of camelCase first which didn't require adding a dependency, but apparently that is being phased out elsewhere in Gutenberg in favor of change-case.
But adding our own function is easy enough and it can be trimmed a bit more to our use-case, so that's fixed in 990f0a1.
Member
|
Thanks for throwing this up @ajlende 👍 I've run out of time to give it a proper test this week. I don't think we need to rush things before 6.1, given that things are working, but the optimizations look good. 🍺 I was wondering if there were any opportunities to unit test the new functions (?) I'll try to take it for a spin over the weekend or next week. |
Contributor
Author
The only thing that I would be concerned about is that the type definition for
There were already tests written for border, and the refactor produces exactly the same output as before. gutenberg/packages/style-engine/src/test/index.js Lines 99 to 148 in 990f0a1 I pushed a change adding doc comments explaining them which should help if something isn't clear. |
ramonjd
approved these changes
Aug 29, 2022
Member
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
Thanks for the work here. Smoked tested in the editor. Tests pass. LGTM 🍺
Contributor
|
Hey folks! Came across this PR via #44406, which was marked for backporting into Core. I have a question related to this comment:
So it looks like |
Contributor
Author
Hmm, you're right. How are those being built? When I run
Is it important that backwards compatibility be maintained between WordPress versions for the types? I was assuming yes when I brought that up, but I may have been overthinking things and it may not be that much of a problem if folks are using types from the npm version. |
Contributor
Author
It looks like style-engine wasn't included in the top-level references so the only way they would be built is if you were running |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I did some refactoring to the border styles as I was trying to make sense of it.
I was able to reduce the signature of the generate function to just style and options like it was before to avoid some confusion that I had in #43526 (comment).
I did make some breaking changes with the type signature, but with the style engine not officially in core WordPress yet, I'm hoping that will still be okay.