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

Apply styling through JavaScript (remove typeahead.css) #15

Closed
wants to merge 8 commits into from

Conversation

jharding
Copy link
Contributor

  • Only need to load typeahead.js now, no more typeahead.css.
  • No longer use tt-is-open and tt-is-empty classes.
  • Use div instead of ol and li.

@jharding jharding mentioned this pull request Feb 21, 2013
@timtrueman
Copy link
Contributor

Looks good to me

Jake Harding added 3 commits February 22, 2013 19:03
…typeahead-css

Conflicts:
	Gruntfile.js
	src/css/typeahead.css
	src/dropdown_view.js
	src/input_view.js
	src/typeahead_view.js
	test/typeahead_view_spec.js
@jlogsdon
Copy link

Why are these changes coming in? Now if I want to override the default styles I have to use !important in my stylesheets. And semantically an ol makes more sense than a div as it's showing a list of suggestions.

Personally I would not use this project if these changes went in, but I know I'm one person so I'd love to hear what other people have to say about this.

@jharding
Copy link
Contributor Author

Main reason why this change is coming in (well, being considered) is because I think it makes typeahead.js easier to use.

Which styles are we setting in this pull request that you'd want to override? I have a hard time imagining you'd want to override any of them as they're mostly for positioning the hint and the dropdown menu, but maybe there's a use case I'm not seeing.

BTW, we currently use !important in typeahead.css. This pull request will actually make it easier for you to override the styles we set as you won't have to worry about rule ordering or selector specificity issues.

Also, we'll be setting fewer styles after this pull request gets merged in, making typeaheads a tad bit easier to customize. One of the reasons why we'll be setting fewer styles is because of the switch from ol and li elements to span and div elements. Having to account for the default styles of ol elements was hurting the flexibility of typeahead.js. Yes, this is less semantic, but I think it's more pragmatic.

Hopefully this addresses your concerns. If you still have issues with this pull request, please voice them.

@jlogsdon
Copy link

Thanks for the speedy reply. You make valid points, sir. I hadn't thought about the styling issues related to ol and li.

Keep up the good work!

@jharding jharding mentioned this pull request Mar 4, 2013
@jharding
Copy link
Contributor Author

jharding commented Mar 4, 2013

Merged into integration-0.9.0.

@jharding jharding closed this Mar 4, 2013
jharding referenced this pull request Mar 19, 2013
…head.js into integration-0.9.0

Conflicts:
	Gruntfile.js
	dist/typeahead.css
	dist/typeahead.min.css
	package.json
	src/dropdown_view.js
	src/typeahead.js
	test/dropdown_view_spec.js
	test/typeahead_view_spec.js
@jimmynotjim
Copy link

It may make typeahead easier to drop into a project, but realistically, anyone that can understand how to apply the plugin knows how to apply layout styles. Even though you had essentially the same styling before with the !important, it was easy enough for end users to not include them and write their own sheet. This made them merely suggestions rather than rules.

Forcing the end user to overwrite your preferred styles with '!important' and restyling the same element multiple times is bad technique in my eyes, especially when the core functionality isn't reliant on layout styles (as it might be in a slideshow plugin for example).

@jharding
Copy link
Contributor Author

Sorry for the delay in getting back to you, life's been hectic lately 😫

What do you consider the core functionality of typeahead.js to be? I think if I know your answer to that question, I'll be able to address your concerns more appropriately.

@jimmynotjim
Copy link

No worries, we all have crazy times.

In my eyes, the core functionality is the returning search results for the query. What happens after that should be up to the developer using it.

For example, I agree with a reply you made on another issue about typeahead not being a select menu plugin. I was bummed at first because that is exactly what I was trying to do with it, but then I thought about what would be required once you opened that box and how typeahead would just become the same thing as Select2. Yet if we want to, we can write some extra scripting with the selected callback to make it act like a select menu.

@jharding
Copy link
Contributor Author

Ah alright so that's what I was hoping you'd say. That functionality is provided by the dataset component which we're planning on creating an API for (see #95). So in theory, you'll eventually be able to do something like this:

var datset = $.typeahead.Dataset({ /* ... */ });

dataset.getSuggestions(query, function(err, suggestions) {
  // now you can render suggestions from typeahead.js however you'd like
});

Considering the dataset component and its dependencies account for probably less than half of the source code, we'll probably want to generate something like typeahead_dataset.js so devs like yourself can take advantage of this functionality without having to load all of the code for the UI components.

@jimmynotjim
Copy link

Nice, now that makes a lot of sense. I was going to suggest an option when initializing to skip over the styles, but this is even better. Carry on :)

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

Successfully merging this pull request may close these issues.

4 participants