-
Notifications
You must be signed in to change notification settings - Fork 0
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
receipts view #15
receipts view #15
Conversation
|
with: on-click, disabled
|
||
@customElement('iaux-monthly-giving-circle') | ||
export class MonthlyGivingCircle extends LitElement { | ||
@property({ type: String }) patronName: string = ''; | ||
|
||
@property({ type: Array }) receipts = []; |
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.
This can become
@property({ type: Array }) receipts: aReceipt[] = [];
so TS knows the type of data its working with and help the developer out during development.
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.
thanks for catch, will update. i will keep class and decorate this list
<th class="amount">Amount</th> | ||
<th class="action">Action</th> | ||
</tr> | ||
${this.receipts.length |
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.
Minor, but to make this render()
method a bit easier to read, I might split out the receipts list view, ie:
${this.receipts.length > 0 : this.receiptsListView : html`<p>No recent donations found</p>`}
private receiptsListView(): TemplateResult {
return html`...`
}
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 did want to show the table row markup inside the context of the table draw function. there aren't any other views to switch from at this level so i think it makes sense to keep in-line. 👍
src/presentational/iaux-button.ts
Outdated
<button | ||
?disabled=${this.isDisabled} | ||
@click=${() => { | ||
if (this.isDisabled) { |
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.
Do you need this disabled check? The ?disabled
above should be making it unclickable, no?
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.
the disabled check ensures the button does not re-fire.
src/presentational/iaux-button.ts
Outdated
export class IauxButton extends LitElement { | ||
@property({ type: Boolean, reflect: true }) isDisabled = false; | ||
|
||
@property({ type: Function }) clickHandler: Function | 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.
Is type: Function
valid? I get an error when I tried using that. I think it would be type: 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.
To make this callback similar to an HTML button and help the developer know what the clickHandler
looks like, I might make this type as (e: Event) => void
, ie:
@property({ type: Object }) clickHandler?: (e: Event) => void;
...
<button @click=${(e: Event) => { this.clickHandler(e) }>
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.
you know, that's from copilot 😅 i wanted to believe
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.
thanks again for the feedback. i've got a good fix around this. we aren't bubbling up the click event, but i'll take a look at switching to passing event.
|
||
if (this.clickHandler) { | ||
this?.clickHandler(this); | ||
} |
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.
The this
isn't optional so you shouldn't need the question mark.
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.
clickhandler can be undefined
|
||
type receiptEmailStatus = { | ||
id: string; | ||
emailStatus: 'success' | 'fail' | 'pending' | ''; |
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.
What does a status of ''
mean?
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.
empty string. i wanted to use same type and default to falsey.
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.
thanks so much for the care @jbuckner and @nsharma123 🙌
|
||
if (this.clickHandler) { | ||
this?.clickHandler(this); | ||
} |
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.
clickhandler can be undefined
src/presentational/iaux-button.ts
Outdated
<button | ||
?disabled=${this.isDisabled} | ||
@click=${() => { | ||
if (this.isDisabled) { |
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.
the disabled check ensures the button does not re-fire.
|
||
type receiptEmailStatus = { | ||
id: string; | ||
emailStatus: 'success' | 'fail' | 'pending' | ''; |
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.
empty string. i wanted to use same type and default to falsey.
<th class="amount">Amount</th> | ||
<th class="action">Action</th> | ||
</tr> | ||
${this.receipts.length |
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 did want to show the table row markup inside the context of the table draw function. there aren't any other views to switch from at this level so i think it makes sense to keep in-line. 👍
https://internetarchive.github.io/iaux-monthly-giving-circle/pr/pr-15/demo/