-
Notifications
You must be signed in to change notification settings - Fork 55
feat(test): add shorthand property conformance tests #112
feat(test): add shorthand property conformance tests #112
Conversation
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
=======================================
Coverage 88.37% 88.37%
=======================================
Files 45 45
Lines 757 757
Branches 109 109
=======================================
Hits 669 669
Misses 85 85
Partials 3 3
Continue to review full report at Codecov.
|
test/utils/uncapitalize.ts
Outdated
@@ -0,0 +1,9 @@ | |||
const uncapitalize = (str?: string): string => { |
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.
_.lowerFirst()
?
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.
actually, it turns out that this functionality is not needed, removed this altogether. Thanks!
src/components/Button/Button.tsx
Outdated
const { content, icon, iconPosition, type } = this.props | ||
const iconIsAfterButton = iconPosition === 'after' | ||
|
||
const iconProps = |
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 this needed?
Can't you just call something like:
Icon.create(this.props.icon, {
defaultProps: { /* xSpacing and others */ },
})}
Shorthand factory can handle string inherently.
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.
yes, lets do this way, thanks!
CHANGELOG.md
Outdated
|
||
### Features | ||
- Add Menu `iconOnly`, MenuItem `iconOnly` and `icon` props @miroslavstastny ([#73](https://github.com/stardust-ui/react/pull/73)) | ||
- Add conformance tests for component shorthand properties @kuzhelov ([#112](https://github.com/stardust-ui/react/pull/112)) |
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.
Note, we only add entries for public facing changes. Chores, like tests, build, and CI changes do not get changelog entries as they do not affect end users.
@@ -11,6 +11,14 @@ import helpers from './commonHelpers' | |||
import * as stardust from 'src/' | |||
import { felaRenderer } from 'src/lib' | |||
|
|||
type ShorthandTestOptions = { | |||
mapsStringValueToProperty?: string |
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.
grammar nit: In the React community, props
is not expanded to property
. Suggest something like mapsValueToProp
instead. This also follows the associated factory function name, mapValueToProps
, another React community norm set by Redux.
import { MenuBehavior } from 'src/lib/accessibility' | ||
|
||
describe('Button', () => { | ||
isConformant(Button) | ||
isConformant(Button).hasConformantShorthandProperty('icon', Icon, { |
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 we should couple common tests with a chaining syntax like this. Inevitably, we are going to want to use a chained shorthand test without first passing a component to isConformant
. Would prefer simple and explicit imports and separate calls here.
Propose we stick with the original test name of implementsShorthandProp
. The base test is called isConformant
as it tests many baseline standards ensuring the component conforms to them. We don't need to call every test "conformant" when dealing with isolated cases such as "shorthand prop", "className", "accessibility", etc. See previous prop vs property comment.
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.
let me, please, share my thoughts on that.
I don't think we should couple common tests with a chaining syntax like this. Inevitably, we are going to want to use a chained shorthand test without first passing a component to isConformant
actually, it is very hard for me to imagine this situation. Our library is about introducing only conformant components (as this is the main point of it - to ensure this consistency between all the components). Could you, please, suggest some hypothetical case where it might happen? Sorry, cannot come up with any by myself :(
Also, it seems that there are two aspects being mixed a bit. I am not against introducing it as a separate module, so that this functionality will be decoupled and modularized. At the same time, the question of the interface under which this functionality will be exposed to developer (chained calls, direct imports, etc) - this seems to be a separate question. The reason I might prefer chained syntax is that it is much more intuitive for the developer which testing methods she might consider (as well as that this chain, generally, describe the workflow for her):
- first, ensure that provided component is conformant
- then, optionally, ensure shorthand properties are properly handled
- ensure other aspects
And instead of looking on other specs for proper import statements, developer could rely on IntelliSense support:
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.
Responses
Examples of non-conformant components are Portal and Provider. Neither of these will pass the conformance test as they don't render typical UI components yet they still require other common tests. The Provider needs rendersChildren
while the Portal needs hasSubcomponents
and likely handlesAccessibility
.
The list of components that will not pass isConformant
will grow. In SUIR, we currently do not run conformance tests on Ref
, TransitionalPortal
, MountNode
, or Transition
yet all of these do import and run several other common tests.
Note that intellisense is still provided for all common tests when importing:
Proposal
Nonetheless, you raise a good point and perhaps we need to rethink what it means to be "conformant" if we expect many components to not conform.
In SUIR we were focused on making sure common UI components used by users conformed to many guidelines. Overtime, some components fell outside of this scope or had difficulty abstracting good tests around them. Stardust UI implemented conformance tests in largely the same way, but with some subtle differences and more capabilities given our *.info.json
files for each component.
Maybe we trim down our conformance tests to strictly include tests that are applicable to all components. Things like naming and exports. Then add each test that does not apply to all components as chained calls? One upside is that all components can be handed to isConformant
.
One downside to this is that it makes more room for error for most devs when creating most components (common UI components). It is nice to simply say isConformant
and fix errors. However, having to know which chained calls they need to add could be daunting, especially if there are many.
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.
I've left my feedback above. I will approve the PR now so as to not block you all day 👍 Please discuss these points with @miroslavstastny and I'll follow the decision you two agree to.
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.
agree with downsides of segregating tests, those will be especially critical now as we haven't critical mass of components for making a proper judgement. Lets follow the simplest path for now (the reason I wasn't sure about it before is that I wasn't aware of Provider and other components' specifics - thanks for the explanation):
- split
isConformant
and optional tests (such asimplementsShorthandProp
) to separate modules - use direct module imports
We can reconsider this decision in future as we'll have more mature set of components (in case if necessary), so that it will be easier to make all the necessary analysis for these decisions.
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.
See comment on chaining and naming.
…operty-conformance-tests
@@ -0,0 +1,123 @@ | |||
import * as _ from 'lodash' |
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.
existing set of tests has been moved to 'SUI' module - lets discuss whether we do need it, as there are some tests that seems to be not relevant for our library
Adds conformance tests for components' shorthand properties. These tests ensure that
shorthand property is defined for component
string value of shorthand property will be processed
Tests for
icon
property ofButton
component were added as well, to ensure this behavior and fix issues we've had before.