-
Notifications
You must be signed in to change notification settings - Fork 87
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
Conversation
A React component for adding a WordPress Range Control input for setting a query's limit clause
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.
Looks great! There's a few things I'd like changed.
.eslintrc.js
Outdated
], | ||
rules: { | ||
"linebreak-style": 0 | ||
}, |
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 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
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 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.
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.
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.
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 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.
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.
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.
* </InspectorControls> | ||
* ); | ||
* }), | ||
*/ |
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 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.
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.
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?
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.
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.
max={ max } | ||
onChange={ onLimitChange } | ||
/> | ||
); |
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.
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.
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.
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?
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.
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.
expect( onChange ).toHaveBeenCalledWith( 30 ); | ||
} ); | ||
} ); | ||
} ); |
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.
A++ love it :) the only change I'd like to see here is a test for when onChange
is undefined.
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.
will do, although not sure what to expect... i guess null
would be returned
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.
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).
|
||
|
||
* [QueryLimit](components/query/limit.md) - A React component for adding a WordPress Range Control input for setting a query's limit clause | ||
|
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.
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 beAA-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.
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.
will rename and move
webpack.common.js
Outdated
@@ -97,6 +97,9 @@ const config = [ | |||
'data-stores': [ | |||
assets + 'data/index.js', | |||
], | |||
'eventespresso-core-blocks': [ | |||
assets + 'blocks/index.js', | |||
], |
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 don't think this should be in this branch since there is no assets/src/blocks/index.js
in this branch?
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.
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.
This reverts commit d298958.
Problem this Pull Request solves
adds a granular component for setting a query's limit value
How has this been tested
added to the Event List component (not in this branch) and supported by Jest tests
Checklist
esc_html__()
, see https://codex.wordpress.org/I18n_for_WordPress_Developers)