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

startController classname #1843

Closed
danielghost opened this issue Oct 13, 2017 · 8 comments
Closed

startController classname #1843

danielghost opened this issue Oct 13, 2017 · 8 comments

Comments

@danielghost
Copy link
Contributor

https://github.com/adaptlearning/adapt_framework/blob/master/src/core/js/startController.js#L73 - expects a selector rather than a classname, so we may wish to either amend the _className attribute to make this clearer or amend the method to look at classes specifically.

@NayanKhedkar
Copy link
Member

@danielghost could you please elaborate more?

@oliverfoster
Copy link
Member

oliverfoster commented Nov 10, 2017

http://api.jquery.com/is/ uses a css selector tagName.className#id[name=value] the attribute is called _className even though it's not a class name, it's a selector.
@danielghost is making a point about the clarity of the variable name because he and I struggled to set it up correctly without reading the code.

we either need to change the name to _selector or change the code to $html.hasClass(className) really
i quite like @tomgreenfield suggestion #1858 (comment)

@NayanKhedkar
Copy link
Member

Here we provided _className with dot(.) prefix i.e. "_className": ".size-small.touch, .size-medium.touch".
So we can not go for $html.hasClass(className) and also we can't provide multiple classes to hasClass().need to be change documentation also.
What would you prefer @oliverfoster ?

@oliverfoster
Copy link
Member

hasClass doesn't allow for and/or style rules.
So you can't do .size-small.touch, .size-medium.touch which is (size-small && touch) || (size-medium && touch).
I like Tom's suggestion in that it allows the expected behaviour for "className" as well as allowing selectors, with one variable. It make sense because it's simple and satisfies all the requirements with the minimal amount of effort.

var $html = $("html");

if (!className || $html.is(className) || $html.hasClass(className)) {

@NayanKhedkar
Copy link
Member

Ok that's fine @oliverfoster

@NayanKhedkar
Copy link
Member

PR:#1865

@sarveshwar-gavhane
Copy link
Contributor

can some one close this issue ?

@oliverfoster
Copy link
Member

👍

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

No branches or pull requests

4 participants