-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat(pillbox): New "blue pillbox" UI component #354
Conversation
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 like the different options for the pillbox. Maybe for the boxes that have the vertical line separator, there could be an option to put the vertical line equidistant from the edges of the box (e.g., giving the Numerator and Denominator part and the Rate part the same width)
src/app/app.component.ts
Outdated
label: 'Single label, no styling or whatever' | ||
}] | ||
}, | ||
simplestCenterdBold: { |
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.
simplestCenterdBold -> simplestCenteredBold and also on line 74 of src/app/app.component.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.
Thanks, I'll clean up the example code naming/duplication!
I'll ask the UX folks how they feel about this suggestion. It does seem like it'd be a simple enough option to add in the future. At the very least, if they like the idea or see a need for it, I'll add it as a code comment/TODO.
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.
Posted a couple of suggestions...
src/app/app.component.html
Outdated
<app-pillbox | ||
style="display:inline-block" | ||
[contentFirst]=pillboxData.fraction.first | ||
[contentFraction]=pillboxData.fraction.fraction> | ||
</app-pillbox> | ||
<app-pillbox | ||
style="display:inline-block" | ||
[contentFirst]=pillboxData.fraction.first | ||
[contentFraction]=pillboxData.fractionZeros.fraction> | ||
</app-pillbox> |
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 in these 2 examples, contentFirst doesn't actually get used...is that correct?
Would it be better to have a single input, say data
that accepts one of 3 types:
data: PillboxContent[] | PillboxContentPair | PillboxFraction
PillboxContentPair would be something like:
{
first: PillboxContent[],
second: PillboxContent[]
}
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.
That's right. a contentFraction
will completely override the contentFirst
and the example is meant to both illustrate that.
I think in one of my prior commits I tried to make contentFirst
required, and use that technique to accept PillboxContent[] | PillboxFraction
... or maybe it was PillboxContent | PillboxContent[]
but for some reason when I tried to handle the array case, it'd complain that the properties I was trying to set didn't exist. But when I'd attempt debug/breakpoints everything seemed to be there.
I can take another stab in a branch and see if I can replicate the error again.
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 this, would we be able to just pass in one data object?
@@ -132,6 +133,68 @@ export class AppComponent implements OnInit { | |||
|
|||
@ViewChild('uploader') uploader: FileUploadComponent; | |||
|
|||
pillboxData = { |
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.
Expanding on what I mentioned above, maybe you need a PillboxContentType type?
type PillboxContentType = PillboxContent[] | PillboxContentPair | PillboxFraction
If you had a type, then you can type pillboxData as a key/value pair of PillboxContentType objects and not have to specify <PillboxContent[]> each time (I think...)
It can be simplified to something like:
pillboxData: {[key: string]: PillboxContentType} = {
// PillboxContent[]
simplest: [{
label: 'Single label, no styling or whatever'
}],
// PillboxContentPair
twoLabels: {
first: [{
label: 'First label',
}],
second: [{
label: 'Second label',
}],
},
// PillboxFraction
fraction: {
numerator: 95,
denominator: 96,
rate: 0.95,
rateTooltip: 'Higher rates indicate better performance'
}
}
NOTE: Syntax may not be correct, just providing a rough untested example...
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.
Hmmm... yeah I'll try this first before trying to recreate the error I described with the multiple allowed @Input
types.
Generally functional and ready for review. @mcelestini @dianadean In the meantime, I'll get some code comments up and wrap unit tests. |
// TODO: | ||
// - code/function comments | ||
// - innerHTML |
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.
nitpicky but can we remove the TODO comment before commiting?
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.
Yeah, plan to remove those as I wrap comments and tests - in progress 👍
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 good to me.
var left: PillboxColumnContent; | ||
var right: PillboxColumnContent; | ||
|
||
if ("top" in pair.left) { |
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 this be in single quotes here and Line 126?
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.
oooh, nice catch - thx, change inbound
## [5.5.0](v5.4.0...v5.5.0) (2023-01-27) ### Features * **pillbox:** New "blue pillbox" UI component ([#354](#354)) ([ef1a691](ef1a691))
🎉 This PR is included in version 5.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
TODO:Handleid
... pass down totooltips
- n/a see code commentsNot sure why the vertical spacing happens like this yet... but here's some of the examples.
Maybe handling input as arrays of PillboxContent, and requiringcontentFirst = null
when usingcontentFraction
is a slightly goofy API. But this is where I landed for now without introducing enums or bespoke objects withupper
andlower
PillboxContent props.Addressed request from Diana and Marc, which made the API less clunky. Also cleaned up the HTML and
ng-template
s therein.