-
Notifications
You must be signed in to change notification settings - Fork 3
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
Use custom events for inputs #42
Conversation
Codecov Report
Continue to review full report at Codecov.
|
f22e02b
to
8146363
Compare
@Prop() required?: boolean; | ||
@Prop() onChange: (e: UIEvent) => void; | ||
@Event() onInputChange: EventEmitter; |
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.
At first, I was hoping we could pass an eventName
as a prop to the component, and use that here. But the way the custom events work, this more or less has to be a hard-coded value.
So instead, I have a generic onInputChange
event being submitted so we can re-use these inputs everywhere.
On the listener side, it’s accepting all these events, but filtering out the ones it doesn’t care about. So basically, if we don’t care about listening to certain inputs we just don’t create a listener for them.
return null; | ||
} | ||
} | ||
} |
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 started out trying to make changes in this file, then I found myself passing so many props back-and-forth it just seemed simpler to delete it 🤷♂️.
It made plan-details.tsx
longer, but I’d like to refactor that in a followup PR to abstract a lot of the display logic into utility functions, and make that component far easier to read.
if (!e.target) return; | ||
const { value } = e.target as HTMLSelectElement; | ||
this.onInputChange.emit({ name: this.name, value }); | ||
}; |
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.
In the event details, we’re submitting the name
of the input so that above in the listener, we can filter out the events with names we don’t care about.
@Event() onInputChange: EventEmitter; | ||
@Watch('defaultValue') watchHandler(newVal: string) { | ||
this.onInputChange.emit({ name: this.name, value: newVal }); | ||
} |
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.
@Watch
is necessary here only because of the plan change.
The defaultValue
is deterministic, and won’t ever update as the user changes input values. It will change, however, if a user switches plans because each plan can have its own defaults. So this is basically a way of emitting a “reset” event to the listener when the plan 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.
I was totally wondering if @Watch
members allowed you to reference this
, because the docs aren't really clear on that. 💯
|
||
@Component({ | ||
tag: 'mf-select', | ||
styleUrl: 'mf-select.css', | ||
scoped: true, | ||
}) | ||
export class MfSelect { | ||
@Prop() options: Option[] = []; | ||
@Prop() selectedValue?: Value; | ||
@Prop() defaultValue?: 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.
Borrowed the defaultValue
from React. Technically, this works like value
, and can overwrite the user’s changes. But the way it’s passed down, it’ll never change so it should allow the user to modify inputs freely (kinda like how HTML works—value
only determines the default. Actually there have been discussions to change this in React)
8146363
to
67ee295
Compare
describe(`<plan-details>`, () => { | ||
it('sets its initial features correctly', async () => { | ||
const page = await newE2EPage({ html: `<plan-details />` }); | ||
await page.$eval('plan-details', (elm: any) => {}); |
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 doing anything right now?
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.
Oh I was going to write some tests then got distracted trying to refactor too much. I want to pull out some of that display logic and test it separately, but there are definitely still tests I can still write here. I’ll add those.
67ee295
to
2969a78
Compare
storage: 5, | ||
}); | ||
}); | ||
}); |
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.
nice, it gets picked up in the code coverage too :)
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.
💯
}; | ||
// TODO: replace this with pricing calculation call | ||
console.log(this.features); | ||
} |
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.
@sslotsky alternative to @Listen
. Seems a bit cleaner because it’s tied directly to the inputs and we no longer do the filtering like you suggested. WYT?
62e5d4b
to
ef5db1a
Compare
|
||
// TODO: extract these into utils/ to be tested | ||
getBooleanDefaultValue(value: Catalog.FeatureValueDetails): boolean { | ||
return value.label === 'true' || false; |
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 the || false
actually needed?
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.
Ah not anymore. In our spec, confusingly, feature.value
may sometimes be missing, and it’s honestly no wonder why we’ve had such a hard time with data. The way this got refactored now, though, I’m handling the case where feature.value
isn’t defined earlier so this is no longer needed.
min={min} | ||
name={feature.label} | ||
onUpdateValue={({ detail: { name, value } }: CustomEvent) => | ||
this.setFeature(name, value) |
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.setFeature
ever called directly, or are we always receiving the detail
object and then destructuring it before calling setFeature
? If we never call it directly, could we just redefine the setFeature
function to accept the detail
object?
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.
We could; in my brain it just seemed confusing to have event-specific params passed to a method that is setting internal state. But yeah we’re not testing this or calling it elsewhere (nor do I plan to), so just a naming opinion, I guess. I can rename that function & let it handle custom events.
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.
Life's a garden, baby, dig it!
A couple small, non-blocking suggestions.
Reason for change
Use Stencil’s Events API to listen to feature input changes, and keep track of latest state in parent component (for use in pricing API, which will come in a followup PR; this is just about tracking the state for now)
Why is this better than using state?
I really like being able to use uncontrolled inputs for performance, as well as minimize the amount of state & prop drilling.
Testing
I didn’t add tests to this PR. While working on it, I had an idea for testing that would require a refactor. I’d rather add tests in a followup PR that (hopefully) cleans up a lot of the custom plan logic. I don’t think I can necessarily reduce the amount of logic there, but I had some ideas on how to at least make it easier to read, and easier to test.