Skip to content
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

Use of bodyContainer + multiple classNames causes an error #300

Closed
alasdairhurst opened this issue Feb 9, 2018 · 6 comments
Closed

Use of bodyContainer + multiple classNames causes an error #300

alasdairhurst opened this issue Feb 9, 2018 · 6 comments
Labels

Comments

@alasdairhurst
Copy link
Contributor

Version 2.2.0

Steps to reproduce

Use the component with className="typeahead"
Get the instance of the component and call blur() on it
Now call focus on the element

Expected Behavior

No errors occur

Actual Behavior

An error is thrown:
image

The issue is here:

container.classList.toggle(className, !!className);

It's passing too much to the "toggle" function as seen here:
image

@ericgio
Copy link
Owner

ericgio commented Feb 11, 2018

This doesn't repro for me in the examples (2.3.0). Can you please provide additional information?

@alasdairhurst
Copy link
Contributor Author

alasdairhurst commented Feb 11, 2018

Looks like I was using 2.3.0 all along (I assumed your github pages was using the latest version when i started using the component but it says 2.2.0)

I will try to create a fiddle or something to reproduce this. Just FYI i don't think this can be reproduced in the examples page since the "focus" api example doesn't have a custom className prop.

@alasdairhurst
Copy link
Contributor Author

https://jsfiddle.net/alasdairhurst/mgwLfodx/1/

Ah, figured it out. We don't even have to use the APIs to trigger it. bodyContainer + className with spaces (I.e multiple classes) causes the issue when the input is focused.

@ericgio ericgio changed the title Focus API broken when using custom className Use of bodyContainer + multiple classNames causes an error Feb 12, 2018
@ericgio
Copy link
Owner

ericgio commented Feb 12, 2018

Thanks for the repro fiddle, I'll have a look.

@ericgio
Copy link
Owner

ericgio commented Feb 12, 2018

This should be fixed as of v2.3.1

@ericgio ericgio closed this as completed Feb 12, 2018
@alasdairhurst
Copy link
Contributor Author

alasdairhurst commented Feb 12, 2018

@ericgio What was the reason for appending the custom class name to the body? I don't see any reason why this should be done. I noticed with the fix in place, it caused some issues:

I scope my typeahead styles to ".mycustomtypeahead", but i need the menu showing in the body. That's ok - i'll just make a custom menu and set the class on that. But with this functionality, the .mycustomtypeahead class will be applied to the body of my page, causing the styles set on that class to be applied to everything on the page.

There are a few workarounds such as .rbt.mycustomtypeahead or :not(.rbt-body-container) but something doesn't seem right.

At the very least, i'd make it clear in the docs that any classes set on the typeahead component could be set on the body as well, but i'm questioning the need.
Now, it could be just for overall scoping, but i'd say using .rbt-menu for any menu selections would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants