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

Copy the html instead of text from header to button ? #21

Closed
oilvier opened this Issue Apr 12, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@oilvier

oilvier commented Apr 12, 2017

Hi,

I was just wondering if there's a reason to copy the text content of the header to the button, instead of the HTML ?

var $button = this.options.button.clone().text($header.text());

It prevents the potential styling applied on the original header element (like the use of tags (strong, span, etc...) or graphics (SVG icons)) to be replicated to the new button element.

But maybe there's an issue I don't see in keeping the HTML ?

@nico3333fr

This comment has been minimized.

Show comment
Hide comment
@nico3333fr

nico3333fr Apr 20, 2017

Owner

Hi,

to be honest, it is more a habit coming from my work: I don't use often font icons, and most of the time I prefer styling directly on the button generated. :)

The only risk that I see is to have bad-formed HTML in the button. II guess I will have to test if there is a difference, and if not, maybe it could be an option of the plugin ? (tell me what you think about it)

Cheers,
Nicolas

Owner

nico3333fr commented Apr 20, 2017

Hi,

to be honest, it is more a habit coming from my work: I don't use often font icons, and most of the time I prefer styling directly on the button generated. :)

The only risk that I see is to have bad-formed HTML in the button. II guess I will have to test if there is a difference, and if not, maybe it could be an option of the plugin ? (tell me what you think about it)

Cheers,
Nicolas

@nico3333fr nico3333fr self-assigned this Apr 20, 2017

@nico3333fr nico3333fr added the question label Apr 20, 2017

@oilvier

This comment has been minimized.

Show comment
Hide comment
@oilvier

oilvier May 6, 2017

Yes, it could be an option like "generatedContent: text / html"

oilvier commented May 6, 2017

Yes, it could be an option like "generatedContent: text / html"

@kotyy

This comment has been minimized.

Show comment
Hide comment
@kotyy

kotyy May 19, 2017

I've been using this plugin in a lot of sites recently. Thank you for developing and maintaining it. It certainly makes my life easier. :)

I'd like to request this feature, too. I can't think of any reason this shouldn't be allowed from a accessibility or semantics perspective, and the spec does allow for phrasing content within buttons.

Changing the line mentioned in oliver's comment, however introduces another problem. clickButtonEventHandler assumes that the target is the button element. If you click on an element within a button, it is the target.

My quick fix was to modify the first line of clickButtonHandler to be

var $target = $(e.target);
var $button = $target.is('button') ? $target : $target.parent();

kotyy commented May 19, 2017

I've been using this plugin in a lot of sites recently. Thank you for developing and maintaining it. It certainly makes my life easier. :)

I'd like to request this feature, too. I can't think of any reason this shouldn't be allowed from a accessibility or semantics perspective, and the spec does allow for phrasing content within buttons.

Changing the line mentioned in oliver's comment, however introduces another problem. clickButtonEventHandler assumes that the target is the button element. If you click on an element within a button, it is the target.

My quick fix was to modify the first line of clickButtonHandler to be

var $target = $(e.target);
var $button = $target.is('button') ? $target : $target.parent();
@chrisgrabinski

This comment has been minimized.

Show comment
Hide comment
@chrisgrabinski

chrisgrabinski Jun 22, 2017

@kotyy Thanks for this great solution. I tried it out and ran into one more, tiny problem that might occur if – for example – you were to use encapsulated phrasing content (i.e. span within span). It's ugly but some designs might call for it. :(

accordion-example

Using jQuery's .closest() instead of .parent() makes sure that the button will always be targeted by the clickButtonHandler.

var $target = $(e.target);
var $button = $target.is('button') ? $target : $target.closest('button');

chrisgrabinski commented Jun 22, 2017

@kotyy Thanks for this great solution. I tried it out and ran into one more, tiny problem that might occur if – for example – you were to use encapsulated phrasing content (i.e. span within span). It's ugly but some designs might call for it. :(

accordion-example

Using jQuery's .closest() instead of .parent() makes sure that the button will always be targeted by the clickButtonHandler.

var $target = $(e.target);
var $button = $target.is('button') ? $target : $target.closest('button');
@nico3333fr

This comment has been minimized.

Show comment
Hide comment
@nico3333fr

nico3333fr Jun 24, 2017

Owner

This is great, I'm finishing some tests and the fix will be release this week-end, thanks to all of you. You're all great 👍

Owner

nico3333fr commented Jun 24, 2017

This is great, I'm finishing some tests and the fix will be release this week-end, thanks to all of you. You're all great 👍

@nico3333fr

This comment has been minimized.

Show comment
Hide comment
@nico3333fr

nico3333fr Jun 25, 2017

Owner

Hi there,

the issue has been fixed in 1607dbf .

The documentation has been updated https://a11y.nicolas-hoffmann.net/accordion/ (also in Readme with a special example ;) https://github.com/nico3333fr/jquery-accessible-accordion-aria/blob/master/README.md#all-options-of-the-plugin )

Thanks all for your help on it, really appreciated. 👍

Owner

nico3333fr commented Jun 25, 2017

Hi there,

the issue has been fixed in 1607dbf .

The documentation has been updated https://a11y.nicolas-hoffmann.net/accordion/ (also in Readme with a special example ;) https://github.com/nico3333fr/jquery-accessible-accordion-aria/blob/master/README.md#all-options-of-the-plugin )

Thanks all for your help on it, really appreciated. 👍

@nico3333fr nico3333fr closed this Jun 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment