patternfly / angular-patternfly

This repo contains instructions and the code for a set of Angular 1 components for the PatternFly project.
http://www.patternfly.org/angular-patternfly/
MIT License
122 stars 90 forks source link

Notification drawer example dropdown kebab broken #759

Closed skateman closed 6 years ago

skateman commented 6 years ago

I was playing with the notification drawer example and found two issues, in notification-body.html:

dtaylor113 commented 6 years ago

...This causes the kebab to activate after a second click only.

Hi @skateman, sorry, I'm not seeing this behavior? The kebab opens on the first click for me (using Chrome)? image

skateman commented 6 years ago

@dtaylor113 oh, you're probably not including the original bootstrap JS on the page. I spend an hour with @Hyperkid123 when we found why it wasn't working in MiQ :laughing:

dtaylor113 commented 6 years ago

Hi @skateman, ok I removed data-toggle="dropdown" from the <button/> in notification-body.html for the Notification Drawer ngDoc example and the kabob menu continues to work on a single click.
<button/> in notification-body.html still has uib-dropdown-toggle which I believe is the correct angular-ui bootstrap attribute.

I'll work on dropdownKebabRight ID issue next. Thanks, Dave

skateman commented 6 years ago

@dtaylor113 yes, the uib-dropdown-toggle is okay, I fixed the attribute to the data-toggle above, sorry and thanks

dtaylor113 commented 6 years ago

I fixed the attribute to the data-toggle above, sorry

Sorry, where did you fix it?

skateman commented 6 years ago

@dtaylor113 in the PR description I had dropdown-toggle instead of data-toggle="dropdown"

dtaylor113 commented 6 years ago

Hi @skateman, do you need id="dropdownKebabRight"? I removed it from notification drawer example, and the example and our unit tests continue to work correctly.

skateman commented 6 years ago

Not sure about the aria- prefix on the dropdown content, it has probably some semantic value. Ideally here you would have to generate a dynamic ID for each notification as we do it here. I know it's not pretty, but it would be nice if we could prevent people from copy-pasting bad stuff (we have a bunch of copy-pasted PF examples that cause duplicate ID issues and that makes QE cry sometimes).

dtaylor113 commented 6 years ago

I'm still not sure that example generates a unique id across all kabob action dropdowns. It's based on notification.id, where it should be something like 'dropdownKebabRight-{{ notification.id }}-{{ notification.action.id }}' because there may be mutliple actions per notification.

skateman commented 6 years ago

Yup, but only one dropdown per-notification and the ID belongs to the dropdown.

dtaylor113 commented 6 years ago

ah, yes, correct, it's been a while :-)