search

HealthCatalyst / Fabric.Cashmere

Health Catalyst’s comprehensive design system.
http://cashmere.healthcatalyst.net
Apache License 2.0
66 stars 76 forks source link

hc-select doesn't support optgroup elements. #1188

Closed jayols closed 4 years ago

jayols commented 4 years ago

Summary

Trying to use optgroup elements to group options within an hc-select and it doesn't appear to work - just renders an empty list.

Reproduction

https://stackblitz.com/edit/cashmere-optgroup-select?file=src/app/select-standard/select-standard-example.component.html

Additional context

Cashmere v6.7.1

andrew-frueh commented 4 years ago

Great suggestion, Jayson! We hadn't heard of anyone using option groups up until now, but we should definitely support it. I'll make this a high priority to get a fix in early next week.

jayols commented 4 years ago

Thanks Andrew! This will definitely help because we have a couple features in our CM app that uses grouped selects and they are using a rather convoluted method to render the options in a similar manner (but involves more code and css). Using an optgroup element would greatly simplify both of those implementations.

From: Andrew Frueh notifications@github.com Sent: Thursday, March 5, 2020 12:23 PM To: HealthCatalyst/Fabric.Cashmere Fabric.Cashmere@noreply.github.com Cc: Jayson Olson jayson.olson@healthcatalyst.com; Author author@noreply.github.com Subject: [EXTERNAL] Re: [HealthCatalyst/Fabric.Cashmere] hc-select doesn't support optgroup elements. (#1188)

Great suggestion, Jayson! We hadn't heard of anyone using option groups up until now, but we should definitely support it. I'll make this a high priority to get a fix in early next week.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/HealthCatalyst/Fabric.Cashmere/issues/1188?email_source=notifications&email_token=AJZQD5AIQ76A5LLILBDUMCLRGAC2NA5CNFSM4LCRZP32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN6YMWQ*issuecomment-595428954__;Iw!!AdoWyQ-9Vto!2WWyabRP98QF0mNaVjiu1nwqpE4KUBy2jIf1FJiEoNpx1neJ6pf06i3VV6eXzIvcb09vbQ$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AJZQD5BBASPAJVR7CYUCJ7LRGAC2NANCNFSM4LCRZP3Q__;!!AdoWyQ-9Vto!2WWyabRP98QF0mNaVjiu1nwqpE4KUBy2jIf1FJiEoNpx1neJ6pf06i3VV6eXzIs_W3O2ZA$.

CAUTION: This email originated from outside of Health Catalyst. Do not click links or open attachments unless you recognize the sender and know the content to be safe.

andrew-frueh commented 4 years ago

Hey @jayols - this was actually an easier fix than I thought. We were unnecessarily filtering ng-content in the template to only options. So just stripping that out allows for optgroups to work fine. I updated one of the examples on Cashmere to demo the use as well.

@Techgeekster if you can take a look at the PR - #1190 - I'll get a patch out tomorrow.

jayols commented 4 years ago

Some additional on this – if possible, it would be nice if a related bug ticket (setting value to object) could also be fixed as we currently have a need for that type of behavior as well.

Thanks, Jayson

From: Andrew Frueh notifications@github.com Sent: Thursday, March 5, 2020 12:23 PM To: HealthCatalyst/Fabric.Cashmere Fabric.Cashmere@noreply.github.com Cc: Jayson Olson jayson.olson@healthcatalyst.com; Author author@noreply.github.com Subject: [EXTERNAL] Re: [HealthCatalyst/Fabric.Cashmere] hc-select doesn't support optgroup elements. (#1188)

Great suggestion, Jayson! We hadn't heard of anyone using option groups up until now, but we should definitely support it. I'll make this a high priority to get a fix in early next week.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/HealthCatalyst/Fabric.Cashmere/issues/1188?email_source=notifications&email_token=AJZQD5AIQ76A5LLILBDUMCLRGAC2NA5CNFSM4LCRZP32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN6YMWQ*issuecomment-595428954__;Iw!!AdoWyQ-9Vto!2WWyabRP98QF0mNaVjiu1nwqpE4KUBy2jIf1FJiEoNpx1neJ6pf06i3VV6eXzIvcb09vbQ$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AJZQD5BBASPAJVR7CYUCJ7LRGAC2NANCNFSM4LCRZP3Q__;!!AdoWyQ-9Vto!2WWyabRP98QF0mNaVjiu1nwqpE4KUBy2jIf1FJiEoNpx1neJ6pf06i3VV6eXzIs_W3O2ZA$.

CAUTION: This email originated from outside of Health Catalyst. Do not click links or open attachments unless you recognize the sender and know the content to be safe.

andrew-frueh commented 4 years ago

That one, unfortunately, is harder because we have to work around an Angular limitation:

https://github.com/angular/angular/issues/26273

In that thread you can see they propose extending SelectControlValueAccessor, but the tricky part is that Select already extends HcFormField so that it can function with hc-form-field. But where there's a will there's a way. I know @joeskeen took a look at this a little while back and hit the same wall I have - but maybe if you're interested, the three of us could put our heads together and see if we can brainstorm some clever way around this.

joeskeen commented 4 years ago

I have an idea that involves composition rather than inheritance. The hc-select could be hcFormField, but have a child that is SelectControlValueAccessor. I'd love to try it out sometime...

Get Outlook for Androidhttps://aka.ms/ghei36

From: Andrew Frueh notifications@github.com Sent: Thursday, March 5, 2020 7:24:42 PM To: HealthCatalyst/Fabric.Cashmere Fabric.Cashmere@noreply.github.com Cc: Joe Skeen joeskeen@outlook.com; Mention mention@noreply.github.com Subject: Re: [HealthCatalyst/Fabric.Cashmere] hc-select doesn't support optgroup elements. (#1188)

That one, unfortunately, is harder because we have to work around an Angular limitation:

angular/angular#26273https://github.com/angular/angular/issues/26273

In that thread you can see they propose extending SelectControlValueAccessor, but the tricky part is that Select already extends HcFormField so that it can function with hc-form-field. But where there's a will there's a way. I know @joeskeenhttps://github.com/joeskeen took a look at this a little while back and hit the same wall I have - but maybe if you're interested, the three of us could put our heads together and see if we can brainstorm some clever way around this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/HealthCatalyst/Fabric.Cashmere/issues/1188?email_source=notifications&email_token=AC5XSQW7CISMICO7J7GFKCDRGBNGVA5CNFSM4LCRZP32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN7ZXLI#issuecomment-595565485, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC5XSQQ5CNTVGK3RFXWONKTRGBNGVANCNFSM4LCRZP3Q.

andrew-frueh commented 4 years ago

I was experimenting last night with making Select extend SelectValueController instead and implement HCFormField...and it looks promising. However I’m still struggling to get a change event to pass an object not a string. Angular converts the object to a value of {1: object}, essentially object to string.

Anyway, I feel like this is doable, just requires some finesse. I’ll schedule 30 mins for next week to brainstorm - and if you guys can help give me some direction I’m happy to do the grunt work.

jayols commented 4 years ago

Sounds good, thanks Andrew!

From: Andrew Frueh notifications@github.com Sent: Friday, March 6, 2020 8:20 AM To: HealthCatalyst/Fabric.Cashmere Fabric.Cashmere@noreply.github.com Cc: Jayson Olson jayson.olson@healthcatalyst.com; Mention mention@noreply.github.com Subject: [EXTERNAL] Re: [HealthCatalyst/Fabric.Cashmere] hc-select doesn't support optgroup elements. (#1188)

I was experimenting last night with making Select extend SelectValueController instead and implement HCFormField...and it looks promising. However I’m still struggling to get a change event to pass an object not a string. Angular converts the object to a value of {1: object}, essentially object to string.

Anyway, I feel like this is doable, just requires some finesse. I’ll schedule 30 mins for next week to brainstorm - and if you guys can help give me some direction I’m happy to do the grunt work.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/HealthCatalyst/Fabric.Cashmere/issues/1188?email_source=notifications&email_token=AJZQD5HKVAXQIJFCR2NTCJ3RGEPENA5CNFSM4LCRZP32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOB5ZXA*issuecomment-595844316__;Iw!!AdoWyQ-9Vto!zHz-f7F_0FKiA76mwJcTM9Ce7fz3Gz_x6INwLJMMNEFxDBlb1vDQcV1bN4-hLNAcHo13cg$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AJZQD5AKCR4DFFF6J4W5MFDRGEPENANCNFSM4LCRZP3Q__;!!AdoWyQ-9Vto!zHz-f7F_0FKiA76mwJcTM9Ce7fz3Gz_x6INwLJMMNEFxDBlb1vDQcV1bN4-hLNAS0BLUjA$.

CAUTION: This email originated from outside of Health Catalyst. Do not click links or open attachments unless you recognize the sender and know the content to be safe.

benjanderson commented 4 years ago

:tada: This issue has been resolved in version 6.7.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

andrew-frueh commented 4 years ago

@jayols - I think we finally cracked the bug on objects in ngValue: #1197. I'll let you know when it's released.