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

Framework: Add viewport module (data module experiments, proof-of-concept) #5206

Merged
merged 7 commits into from
Feb 27, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 22, 2018

This pull request seeks to implement a new generic @wordpress/viewport module to replace and generalize the current isMobile state maintained in edit-post. There are many changes here, some of which tentatively proposed, which could be split out into smaller pull request, but included here as part of the larger story of demonstrating a full-featured proof-of-concept standalone data-exposing module.

Specifically, ideas explored here include:

There's a dramatic simplification of post editor with these changes, as it no longer needs to maintain state about viewport.

Testing instructions:

Verify that there's no regressions in viewport behavior.

Examples:

  1. The "More" menu should only show "Fix Toolbar to Top" option in large viewports
  2. The sidebar should dismiss itself when collapsing from large to small viewport

Remaining tasks:

  • Unit tests
  • Top-level viewport module documentation

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Extensibility The ability to extend blocks or the editing experience labels Feb 22, 2018
*
* @return {Object} Registered store object.
*/
export function registerStore( reducerKey, options ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like it 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yes, much nicer. I think this should be the only public API available. We could turn registerReducer, registerActions and registerSelectors into the implementation detail.

Copy link
Member

Choose a reason for hiding this comment

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

Unless we want to allow other plugins to make modifications to not their own stores.

@aduth
Copy link
Member Author

aduth commented Feb 22, 2018

An example of where this could prove valuable outside of edit-post is in editor. Did you know we're currently rendering the mobile block toolbar on desktop displays?

https://github.com/WordPress/gutenberg/blob/master/editor/components/block-list/block-mobile-toolbar.js

Yes, it's hidden by CSS, but it still incurs wasteful and unnecessary DOM reconciliation.

data/index.js Outdated
*/
export const subscribe = ( listener ) => {
listeners.push( listener );
export const subscribe = ( ...args ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like it a bit less, but not a blocker :) I can see the advantage of factorizing this kind of logic we could need in several places.

Copy link
Member Author

@aduth aduth Feb 22, 2018

Choose a reason for hiding this comment

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

The alternative being:

let lastBlocks;
subscribe( () => {
	const blocks = select( 'core/editor' ).getBlocks();
	if ( blocks !== lastBlocks ) {
		// ... Blocks are known to have changed.
	}
	lastBlocks = blocks;
} );

Aside from being verbose and clumsy, there's another issue here: The selector is called on every single state change to every single registered store. For expensive queries (even memoized), this would be quite resource intensive.

Copy link
Contributor

@youknowriad youknowriad Feb 22, 2018

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but in your example the subscribe logic is run when core/viewport state changes, which is not so great.

Copy link
Member

Choose a reason for hiding this comment

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

Minor not a blocker just a thought: In redux subscribes always receives a listener and there is no "filtering" possibility. In my opinion, I think we should also do the same, subscribe always receives just a listener, so we are consistent with redux. I understand filtering a specific change will be very a common case, so to address we can use: compose( selectorListner(selector), reducerListner( reducer ), subscribe ). This makes clear that some code is always executed for each change, and our functions selectorListner and reducerListner are just helpers to filter out change. Other helpers can be created in the future.

Copy link
Member Author

@aduth aduth Feb 26, 2018

Choose a reason for hiding this comment

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

@jorgefilipecosta This is interesting. At its most generic, it really only requires a "has value changed" higher-order function to encapsulate the behavior from the original alternative. Something like:

subscribe( ifChanged( () => (
	select( 'core/viewport' ).isViewportMatch( '< medium' )
) )( () => {
	const isSmall = isViewportMatch( '< medium' );

	// Collapse sidebar when viewport shrinks.
	if ( isSmall ) {
		dispatch( 'core/edit-post' ).closeGeneralSidebar();
	}
} ) );

Then it becomes a question of: Do we expect this to be such a common pattern that it's still worth building conveniences into the API itself? The above snippet is generic, and not terribly verbose or clumsy, but still more verbose than the changes that had been proposed here. Generalism can be an ideal, but it shouldn't deter us from creating conveniences for common patterns.

It's a discussion worth having though:

  • What other subscribe helpers could we imagine?
  • What sorts of things would we expect people to use selector subscriptions for?

To the latter question, I'm finding that this could satisfy any need for a side effect pattern in the data module. In fact, taken to its extreme, we might be able to replace all of our own internal side effects with this approach, representing the side effects as a result of a change in state. Can all of the current and foreseen side effects be stated this way, or is access to the raw actions, even those not resulting in a change, necessary? It'd require some investigation, but I'm inclined to say that we could and should try to model side effects on changes in state. This might help remedy some of the disorganization that has resulted in the various effects.js files. At a higher-level, it might even be seen as the data equivalent of React's visual declarative UI, where "props" in this case are the selector result(s).

Copy link
Member Author

Choose a reason for hiding this comment

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

As a testament to the clumsiness of #5206 (comment), this example code snippet inadvertently introduces an infinite loop 😏 (Dispatch causes subscribe to re-invoke before lastBlocks is assigned)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for thoughts and analysing this, it is good that we will be able to discuss this specifc part outside of the scope of the rest of the changes 👍

subscribe( 'core/viewport', [ 'isViewportMatch', '< medium' ], ( isSmall ) => {
// Collapse sidebar when viewport shrinks.
if ( isSmall ) {
dispatch( 'core/edit-post' ).closeGeneralSidebar();
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is a new behavior, how was this handled before? The separation between the mobile sidebar and the desktop sidebar?

Copy link
Member Author

@aduth aduth Feb 22, 2018

Choose a reason for hiding this comment

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

It was previously effected in edit-post/store/reducer.js's handling of the UPDATE_MOBILE_STATE action triggered by enhanceWithBrowserSize.

wp_register_script(
'wp-viewport',
gutenberg_url( 'viewport/build/index.js' ),
array(),
Copy link
Contributor

Choose a reason for hiding this comment

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

missing element, data and components here ;)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Awesome work, didn't test yet but the code is great.

@jorgefilipecosta jorgefilipecosta self-requested a review February 22, 2018 22:27
@aduth aduth force-pushed the add/if-viewport-size branch 2 times, most recently from 1d68420 to be72f5b Compare February 23, 2018 02:22
@@ -1,44 +0,0 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it should be removed with #4777, but my comment was missed.

*
* @return {Function} Higher-order component.
*/
const ifCondition = ( predicate, propName ) => ( WrappedComponent ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this one is cool 👍

data/index.js Outdated
*
* @return {Function} Higher-order function to call with ifTrueFn.
*/
const when = ( predicate ) => ( ifTrueFn ) => ( ...args ) => {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like when and whenWith should be extracted to their own file so we could have only public APIs in this file. Can we also add simple unit tests for both functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Create our own FP utility library? 😄

Copy link
Member

@gziolo gziolo Feb 23, 2018

Choose a reason for hiding this comment

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

I'm fine about pulling ramda and recompose in 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Disappear

Copy link
Member Author

Choose a reason for hiding this comment

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

Since 84c64c8 was removed in rebase, this is no longer relevant.

expect( oneCallback ).toHaveBeenCalled();
expect( twoCallback ).not.toHaveBeenCalled();

oneUnsubscribe();
Copy link
Member

@gziolo gziolo Feb 23, 2018

Choose a reason for hiding this comment

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

Minor: when expect fails unsubscribe methods won't be executed. It shouldn't have impact on other tests though. One way to solve it is to put all unsubscribe methods in an array and iterate over it using afterEach.

let unsubscribes = [];
afterEach( () => {
    unsubscribes.forEach( ( unsubscribe ) => unsubscribe() );
    unsubscribes = [];
} );
it( '...', () => {
    unsubscribes.push( subscribe( 'one', oneCallback ) );
} );

or something like this.

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, that's true. At the same time though, if a test fails, all bets are off. The only issue is if it impacted future failures that caused needless debugging. I'll see about adding lifecycle.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, it might fail not related tests in the future. It’s a minor thing and really hard to implement in a clean way 🙁

Copy link
Member Author

Choose a reason for hiding this comment

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

Since 84c64c8 was removed in rebase, this is no longer relevant. That said, before dropping, I did find it was pretty simple to implement a helper subscribe wrapper:

const unsubscribes = [];
afterEach( () => {
	let unsubscribe;
	while ( ( unsubscribe = unsubscribes.shift() ) ) {
		unsubscribe();
	}
} );

function subscribeWithUnsubscribe( ...args ) {
	unsubscribes.push( subscribe( ...args ) );
}

);
}

MyComponent = ifViewportMatches( '< small', 'isMobile' )( MyComponent );
Copy link
Member

Choose a reason for hiding this comment

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

This one might be confusing because it behaves differently than when it contains only one param. Should we create 2nd HOC component instead? Example:

MyComponent = withViewportProps( { isMobile: '< small' } )( MyComponent );

Copy link
Member Author

@aduth aduth Feb 23, 2018

Choose a reason for hiding this comment

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

This one might be confusing because it behaves differently than when it contains only one param. Should we create 2nd HOC component instead?

I'd totally agree from a consumer's perspective these make sense as two separate components. My only worry is that from the component author's perspective, it's a fair amount of work if we want all "if" HOC to also have a complementary "with".

I might imagine we could leverage some composition here though:

ifViewportMatches = compose( [
	withViewportMatch( '< small' ),
	ifPropsMatch( { isViewportMatch: true }, { omitProps: true } ),
] );

Copy link
Member

Choose a reason for hiding this comment

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

That’s interesting idea to compose two HOCs where both of them might be useful by their own, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Split to separate higher-order components withViewportMatch and ifViewportMatches in fe100d4.

* @return {boolean} Whether viewport matches query.
*/
export function isViewportMatch( state, query ) {
const key = [ '>=', ...query.split( ' ' ) ].slice( -2 ).join( ' ' );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I had to spend some time to understand what's happening here. It might be only me, so feel free to ignore. It might be more code to express it differently, but maybe it would pay off. Example:

const key = query.split( ' ' ).length === 1 ? `>= ${ query }` : query;

I think it will work with the existing tests.

Copy link
Member Author

@aduth aduth Feb 23, 2018

Choose a reason for hiding this comment

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

Minor: I had to spend some time to understand what's happening here.

Yeah, it's one of my favorite "clever" codes that I still find myself leaning on. 😄 I might argue it's a valuable pattern that we can introduce developers to. For example, I've also found value in using it to format a date as MM-DD-YYYY (where the source value of MM and DD can be one or two digits):

[ d.getMonth() + 1, d.getDate() ].map( ( n ) => ( '0' + n ).slice( -2 ) ).concat( d.getFullYear() ).join( '-' )
// "02-23-2018"

Maybe some clarifying comments explaining the behavior?

Copy link
Member

@gziolo gziolo Feb 23, 2018

Choose a reason for hiding this comment

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

const lastTwo = items => items.slice( -2 );
cons key = lastTwo( [ '>=', ...query.split( ' ' ) ] ).join( ' ' );

For me personally, this would help :)

Copy link
Member Author

Choose a reason for hiding this comment

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

For me personally, this would help :)

https://lodash.com/docs/4.17.5#takeRight 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an inline comment and used Lodash's _.takeRight in rebased 217021a.

@gziolo
Copy link
Member

gziolo commented Feb 23, 2018

I like this proposal. 👍

return <WrappedComponent { ...props } />;
};

EnhancedComponent.displayName = getWrapperDisplayName( WrappedComponent, 'condition' );
Copy link
Member

Choose a reason for hiding this comment

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

Should it be getWrapperDisplayName( WrappedComponent, 'ifCondition' );?

Copy link
Member

Choose a reason for hiding this comment

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

We omit prefixes in other places, but we might change this if this is confusing.

Copy link
Member Author

@aduth aduth Feb 26, 2018

Choose a reason for hiding this comment

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

We omit prefixes in other places, but we might change this if this is confusing.

I'm thinking particularly with #5206 (comment) in mind, we might want to do this, since we could have two higher-order components with similar names distinguished on prefix, e.g. withViewportMatch, ifViewportMatches.

Also to the point of #5206 (comment), I'm starting to think maybe we could use a new higher-order component utility in place of the current getWrapperDisplayName to allow for easier HOC composition.

Example:

export default createHigherOrderComponent(
	'ifViewportMatches',
	( query ) => compose( [
		withViewportMatch( { isViewportMatch: query } ),
		ifCondition( ( props ) => props.isViewportMatch ),
	] )
);

Copy link
Member

Choose a reason for hiding this comment

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

@aduth, this is actually a great idea even for the regular HOCs. See:

const ifCondition = ( predicate ) => ( WrappedComponent ) => {
	const EnhancedComponent = ( props ) => {
		if ( ! predicate( props ) ) {
			return null;
		}

		return <WrappedComponent { ...props } />;
	};

	EnhancedComponent.displayName = getWrapperDisplayName( WrappedComponent, 'ifCondition' );

	return EnhancedComponent;
};

vs.

const ifCondition = createHigherOrderComponent(
	'ifCondition',
	( predicate ) => ( WrappedComponent ) => {
		return ( props ) => {
			if ( ! predicate( props ) ) {
				return null;
			}

			return <WrappedComponent { ...props } />;
		};
	},
);

I'm voting to refactor all HOCs to use the helper you proposed and update all prefixes to match what has been discussed here.

Copy link
Member

@gziolo gziolo Feb 27, 2018

Choose a reason for hiding this comment

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

Implementation wise it might be non-trivial task because some HOCs take additional params and require a higher-order function to be involved :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Implementation wise it might be non-trivial task because some HOCs take additional params and require a higher-order function to be involved :)

Yeah, I was thinking about this too. One option is to test whether the argument passed is a class extending Component, but this would mean that all higher-order components would have to be implemented as the full-on class form. Maybe not a big deal to impose this?

*
* @type {Object}
*/
const BREAKPOINTS = {
Copy link
Member

Choose a reason for hiding this comment

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

can we do something similar to what gutenberg/edit-post/store/constants.js does? Use the scss variable loader '!!sass-variables-loader!../assets/stylesheets/_breakpoints.scss';

Copy link
Member Author

Choose a reason for hiding this comment

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

can we do something similar to what gutenberg/edit-post/store/constants.js does? Use the scss variable loader '!!sass-variables-loader!../assets/stylesheets/_breakpoints.scss';

My main issue with this is that the breakpoints are defined in an edit-post stylesheet. Ideally those common SCSS variables don't remain there. The main issue we solve with the variable loader is avoiding repetition, but one of the goals with viewport module is at least to consolidate all viewport-related logic. So hopefully at most we'd have at most two sets to maintain: one in SCSS and one in JS.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it's trade that makes sense to make, and as it is just this cases, it is not hard to maintain 👍

@jorgefilipecosta
Copy link
Member

Awesome work here 👍This seems to work well, in my smoke tests I did not find regressions.

@aduth
Copy link
Member Author

aduth commented Feb 26, 2018

Tracking reference to reducer / selector subscription implementation, as I'm planning to drop it in rebase (and potentially explore separately): 84c64c8

{ storeKey: 'edit-post' }
)( withInstanceId( FeatureToggle ) );
} ) ),
ifViewportMatches( 'medium' ),
Copy link
Member

Choose a reason for hiding this comment

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

Would it make any difference in case ifViewportMatches( 'medium' ) would be composed as the first item here? All HOCs would have to be applied when the source file loads, but it seems like withSelect and withDispatch wouldn't have to be rendered in case the viewport doesn't match. It might only make any difference on the initial render if I process the application flow properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make any difference in case ifViewportMatches( 'medium' ) would be composed as the first item here?

I'll test it, but yes, this seems like it could potentially serve as a (small) optimization.

withSelect( ( select ) => ( {
hasFixedToolbar: select( 'core/edit-post' ).isFeatureActive( 'fixedToolbar' ),
} ) ),
withViewportMatch( { isLargeViewport: 'medium' } ),
Copy link
Member

Choose a reason for hiding this comment

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

It looks really nice. Thanks for taking my feedback into account 👍

const store = applyMiddlewares(
registerReducer( MODULE_KEY, withRehydratation( reducer, 'preferences', STORAGE_KEY ) )
);
const store = registerStore( 'core/edit-post', {
Copy link
Member

Choose a reason for hiding this comment

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

So much nicer 💯

wp_register_script(
'wp-viewport',
gutenberg_url( 'viewport/build/index.js' ),
array( 'wp-element', 'wp-data', 'wp-components' ),
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that it has too many dependencies to move it to @wordpress/packages.

);
}

MyComponent = withViewportMatch( { isMobile: '< small' } )( MyComponent );
Copy link
Member

@gziolo gziolo Feb 27, 2018

Choose a reason for hiding this comment

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

I'm still not sure whether we shoud pick the signature closer to what you proposed before:

MyComponent = withViewportMatch( 'isMobile', '< small' )( MyComponent );

or

MyComponent = withViewportMatch( { isMobile: '< small' } )( MyComponent );

The latter seems to be more flexible, but we don't have any use case in the code that validates we need that capability.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still not sure whether we shoud pick the signature closer to what you proposed before:

In my initial attempt at splitting the components, I'd gone with the first of the two, but upon some reflection I think the object form is better:

  • More familiar in other usage of higher-order components which map to props as an object (e.g. withSelect)
  • Allows for multiple viewport props to be provided (handling multiple cases of viewport size)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, probably is the best to keep it as it is.

*/
const ifViewportMatches = ( query ) => ( WrappedComponent ) => {
const EnhancedComponent = compose( [
withViewportMatch( {
Copy link
Member

Choose a reason for hiding this comment

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

This is amazing, how you composed 2 existing HOCs to create another one 🚀


/**
* Higher-order component creator, creating a new component which renders if
* the viewport query is satisfied or with the given optional prop name.
Copy link
Member

Choose a reason for hiding this comment

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

or with the given optional prop name - this part is no longer applicable.

*
* @type {Object<string,MediaQueryList>}
*/
const queries = reduce( BREAKPOINTS, ( result, width, name ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: setIsMatching might be declared after queries to signal that it depends on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nit: setIsMatching might be declared after queries to signal that it depends on it.

Actually, this was tricky. I'd originally had it as you suggest, but the problem was that when adding the list.addListener( setIsMatching ) below, it would work in that setIsMatching was already declared via variable hoisting, but undefined at the time. addListener happily accepted the undefined value, but would never trigger the callback when it changed.

Copy link
Member

Choose a reason for hiding this comment

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

Got you. I missed that 👍

export function isViewportMatch( state, query ) {
// Pad to _at least_ two elements to take from the right, effectively
// defaulting the left-most value.
const key = takeRight( [ '>=', ...query.split( ' ' ) ], 2 ).join( ' ' );
Copy link
Member

Choose a reason for hiding this comment

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

🙇

* @return {Function} Higher-order component.
*/
const withViewportMatch = ( queries ) => ( WrappedComponent ) => {
const EnhancedComponent = compose( [
Copy link
Member

Choose a reason for hiding this comment

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

We don't need compose in here.

onMobile: isMobile( state ),
} ),
( dispatch, ownProps ) => ( {
export default compose( [
Copy link
Member

Choose a reason for hiding this comment

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

See how compose is implemented in Ramda: http://ramdajs.com/docs/#compose. Lodash allows to pass an array, but in the FP world it is a function that takes any number of functions:

((y → z), (x → y), …, (o → p), ((a, b, …, n) → o)) → ((a, b, …, n) → z)

Copy link
Member Author

Choose a reason for hiding this comment

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

Lodash allows to pass an array, but in the FP world it is a function that takes any number of functions:

Yeah, I don't feel strongly one way or the other, but knowing how arguments works with Function#length and various V8 deoptimizations, I am (apathetically) cautious to avoid it for fear of encouraging its use.

Tangentially related: See classcat vs. classnames where the difference is passing as array vs. arguments respectively, with the former benefitting as such both in performance and bundle size.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to measure the impact in here and add Eslint rule so we could fly with one style 👍
Speaking myself, it’s also tempting to teach Babel to optimize our code, in cases like this. I’m fine with the surrounding array syntax but I’d love to see one style everywhere.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code-wise, I think this one is ready to be merged after some very minor issues are addressed. Great work on this one @aduth 💯

@aduth aduth force-pushed the add/if-viewport-size branch from cc2fb9f to 3db32df Compare February 27, 2018 16:56
@aduth aduth force-pushed the add/if-viewport-size branch from e9af8be to 6e20425 Compare February 27, 2018 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants