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

QueryLimit: A React component for adding a WordPress Range Control input for setting a query's limit clause #484

Merged
merged 17 commits into from
May 29, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module.exports = {
],
'plugins': [
'wordpress',
'jest',
],
env: {
'jest/globals': true,
Expand All @@ -13,9 +14,9 @@ module.exports = {
wpApiSettings: true,
eejsdata: true
},
plugins: [
'jest',
],
rules: {
"linebreak-style": 0
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I know your commit message said you added this linebreak-style for windows but I'm wondering why you are adding this? We've imported the eslint-config-wordpress ruleset which has this:

// Disallow mixed "LF" and "CRLF" as linebreaks
'linebreak-style': ['error', 'unix'],

In other words we're INTENTIONALLY avoiding CRLF in our code. So I'd prefer you did not override this rule. If you're getting linting fails locally, that's something you need to fix locally as the linter is performing as expected. There's many options that you can use to do so, one of them is implement in phpStorm the reading of our .editorconfig file. Here's a github issue that may be useful to you as well: diegohaz/arc#171

Copy link
Contributor

@nerrad nerrad May 29, 2018

Choose a reason for hiding this comment

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

I also know your rule just disables the linebreak style checks. I'd still prefer if you left the rules as is prior to your changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I already have the EditorConfig plugin installed and enabled on my system and my IDE's line endings are (and always have been) set to LF and not CRLF.
I also confirmed that the autocrlf option in my git config was set to true so I'm not sure why these files are appearing as CRLF.

I added the above rule because when I run npm test without it I get lint errors for every single JS file in EE like this:

 2924 problems (2924 errors, 0 warnings)

I can try running that again with the --fix option, but it will result in a lot of files being changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't know what to say then, because what you are seeing does not jive with either what I see locally or what travis sees.

Copy link
Contributor

Choose a reason for hiding this comment

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

as a short term fix for your needs locally, you can run just the js unit-tests without a lint check via npm run test-unit. However, long-term you'll want to look into what is changing the line endings on your machine.

overrides: [
{
files: [ 'test/e2e/**/*.js' ],
Expand Down
3 changes: 3 additions & 0 deletions assets/src/components/index.js
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
// query components
export { QueryLimit } from './query/limit';
// selection components
export { default as EventSelect } from './selection/event-select';
76 changes: 76 additions & 0 deletions assets/src/components/query/limit/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* USAGE:
*
* A React component for adding a WordPress Range Control input
* for setting a query's limit clause
*
* Step 1: import the following external dependencies:
*
* import { __ } from '@eventespresso/i18n';
* import { QueryLimit } from '@eventespresso/components';
* import { PanelBody } from '@wordpress/components';
* import { InspectorControls } from '@wordpress/editor';
*
* Step 2: add a "limit" attribute:
*
* attributes: {
* limit: {
* type: 'number',
* default: 5, (10 is already set as default)
* },
* },
*
* Step 3: wrap the QueryLimit component within
* the InspectorControls and PanelBody components
* (currently shown within a block's edit() function)
*
* edit: ( ( { attributes, setAttributes } ) => {
* const { limit } = attributes;
* return (
* <InspectorControls>
* <PanelBody>
* <QueryLimit
* limit={ limit }
* onLimitChange={
* ( value ) => setAttributes( { limit: value } )
* }
* />
* </PanelBody>
* </InspectorControls>
* );
* }),
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer not to have documentation like this in the component. Having the documentation in our docs folder (as you did) is sufficient or we could adopt the same pattern as GB where there's a README.md located in the same directory that has documentation for the things in that directory. That might work better for js documentation as we remove the entire source directory for release builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an actual reason other than preference?

I added this here so that someone looking through our components folder that wanted to use QueryLimit would know how to do so without having to go searching elsewhere for documentation about it. So maybe having a README.md here would be good as well as the other documentation I provided (ie: just duplicate all block/component documentation so that there are local in place READMEs as well as the docs folder where everything is combined).

We may even want to follow this pattern for PHP and include READMEs within individual folders, so for example the core/services/commands folder could have it's own README.md which would be a duplicate (or link to) the existing documenation at \docs\N--Core-Functionality\command-bus-hexagonal-architecture.md.

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an actual reason other than preference?

One reason to prevent duplication of efforts. If something changes there's multiple places to look. If we're consistent with where extensive documentation is provided then there is no guesswork involved. Another reason is having extensive documentation like this inline means there's more scrolling involved before getting to actual code when working with it which for people occasionally interacting with our code isn't a pain, but personally I find it a significant pain when I have to scroll through big blocks of comments just to get to actual code.

just duplicate all block/component documentation so that there are local in place READMEs as well as the docs folder where everything is combined

I don't think we should duplicate. The docs folder can have a single table of contents that LINKS to the existing js source documentation in the js source folders. We really need to stay away from duplicating the text of documentation because it increases the maintenance overhead.

We may even want to follow this pattern for PHP and include READMEs within individual folders, so for example the core/services/commands folder could have it's own README.md which would be a duplicate (or link to) the existing documenation at \docs\N--Core-Functionality\command-bus-hexagonal-architecture.md.

No, this won't work well for php documentation because we don't have a separate "src" folder that gets removed on releases. So that means we're spreading out all our php documentation all over the place with no easy way to remove those excess files on builds. We can do it with javascript documentation because we do remove the assets/src contents on release builds.


/**
* External dependencies
*/
import { __ } from '@eventespresso/i18n';

/**
* WordPress dependencies
**/
import { RangeControl } from '@wordpress/components';

const DEFAULT_LIMIT = 10;
const DEFAULT_LABEL = __( 'Limit', 'event_espresso' );
const DEFAULT_MIN = 1;
const DEFAULT_MAX = 100;

export const QueryLimit = ( {
onLimitChange,
limit = DEFAULT_LIMIT,
label = DEFAULT_LABEL,
min = DEFAULT_MIN,
max = DEFAULT_MAX,
} ) => {
return onLimitChange && (
<RangeControl
key={ 'query-limit' }
value={ limit }
label={ label }
min={ min }
max={ max }
onChange={ onLimitChange }
/>
);
Copy link
Contributor

@nerrad nerrad May 29, 2018

Choose a reason for hiding this comment

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

Should there be an exception or some human friendly error message when onLimitChange is undefined? I'm on the fence because it will already be clear something isn't working correctly likely.

Copy link
Member Author

Choose a reason for hiding this comment

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

ya I wondered that myself but I wasn't sure what the convention was within GB components.
I don't see any exceptions or errors thrown in other GB components in similar scenarios.

I think it might be acceptable to throw an exception or at the very least log an error to the console, because this component is primarily intended to be used within another component, so any issues would likely only arise during development and will help speed things along. You also mentioned something about GB having some kind of built in notices now, although that may not be as appropriate for this.

Your call though, exception, or console log, or notice, or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

A brief perusal through the GB repo seems to indicate they are doing pretty much what you are doing so let's just leave as is for now. It'll be pretty obvious in development when the component isn't rendered that something isn't working.

};
54 changes: 54 additions & 0 deletions assets/src/components/query/limit/test/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* External dependencies
*/
import { mount } from 'enzyme';

/**
* Internal dependencies
*/
import { QueryLimit } from '../';

describe( 'QueryLimit', () => {
describe( 'Component with no optional parameters passed', () => {
it( 'should have default values', () => {
const onChange = jest.fn();
const wrapper = mount( <QueryLimit onLimitChange={ onChange } /> );
// test range input
const limit = wrapper.find( 'input[type="range"]' );
expect( limit.props().value ).toBe( 10 );
expect( limit.props().min ).toBe( 1 );
expect( limit.props().max ).toBe( 100 );
// and label
const label = wrapper.find( 'label' );
expect( label.text() ).toEqual( 'Limit' );
// test onChange
limit.simulate( 'change', { target: { value: '5' } } );
expect( onChange ).toHaveBeenCalledWith( 5 );
} );
} );
describe( 'Component with all parameters passed', () => {
it( 'should not have default values', () => {
const onChange = jest.fn();
const wrapper = mount(
<QueryLimit
limit={ 25 }
label={ 'No Limit' }
min={ 10 }
max={ 50 }
onLimitChange={ onChange }
/>
);
// test range input
const limit = wrapper.find( 'input[type="range"]' );
expect( limit.props().value ).toBe( 25 );
expect( limit.props().min ).toBe( 10 );
expect( limit.props().max ).toBe( 50 );
// and label
const label = wrapper.find( 'label' );
expect( label.text() ).toEqual( 'No Limit' );
// test onChange
limit.simulate( 'change', { target: { value: '30' } } );
expect( onChange ).toHaveBeenCalledWith( 30 );
} );
} );
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

A++ love it :) the only change I'd like to see here is a test for when onChange is undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

will do, although not sure what to expect... i guess null would be returned

Copy link
Contributor

@nerrad nerrad May 29, 2018

Choose a reason for hiding this comment

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

ya I'm guessing either null or undefined might get returned. It's just a good way to document (via the test) the expectation (in case that ever changes).

13 changes: 13 additions & 0 deletions docs/P--Gutenberg Blocks and Components/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Gutenberg Blocks and Components

### Blocks
The following is a list of Event Espresso Gutenberg Blocks that can be utilized in your site's content:

* [Event Attendees List](blocks/event-attendees.md)

### Components
If you are building your own Gutenberg Block to use within Event Espresso, then the following is a list of additional React components can be utilized:


* [QueryLimit](components/query/limit.md) - A React component for adding a WordPress Range Control input for setting a query's limit clause

Copy link
Contributor

Choose a reason for hiding this comment

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

Gutenberg is a code name and I'm not sure we should use it in our documentation. In the case of the folder structure it means that if we remove the name down the road it will break any direct bookmarks people have made.

So I have a couple suggestions:

  • P--Blocks-and-Components (please use dashes because they are easier to work with for urls).
  • or nest it in the existing AA-Javascript-in-EE folder so it would be AA-Javascript-in-EE/Blocks-and-Components/ I prefer this option because this stuff is mostly js related. However, I'm not going to be sticky on this point because I also see the value of it being a top-level folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

will rename and move

47 changes: 47 additions & 0 deletions docs/P--Gutenberg Blocks and Components/components/query/limit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# QueryLimit

A React component for adding a WordPress Range Control input for setting a query's limit clause

### Step 1
Import the following external dependencies:

```
import { __ } from '@eventespresso/i18n';
import { QueryLimit } from '@eventespresso/components';
import { PanelBody } from '@wordpress/components';
import { InspectorControls } from '@wordpress/editor';
```

### Step 2
Add a "limit" attribute:

```
attributes: {
limit: {
type: 'number',
default: 5,
},
},
```

### Step 3
Wrap the QueryLimit component within the InspectorControls and PanelBody components
(currently shown within a block's edit() function)

```
edit: ( ( { attributes, setAttributes } ) => {
const { limit } = attributes;
return (
<InspectorControls>
<PanelBody>
<QueryLimit
limit={ limit }
onLimitChange={
( value ) => setAttributes( { limit: value } )
}
/>
</PanelBody>
</InspectorControls>
);
}),
```
3 changes: 3 additions & 0 deletions webpack.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ const config = [
'data-stores': [
assets + 'data/index.js',
],
'eventespresso-core-blocks': [
assets + 'blocks/index.js',
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be in this branch since there is no assets/src/blocks/index.js in this branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya so this is what is causing the fail on travis because the npm build step in the job fails with this entry. So removing it will fix that.

},
externals: Object.assign( externals, {
'@eventespresso/components': 'eejs.components',
Expand Down