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

Search articles in group_orders form #180

Merged

Conversation

JuliusR
Copy link
Contributor

@JuliusR JuliusR commented Sep 23, 2013

See #143 for previous discussion.

This adds the possibility to filter articles by name when ordering. The plugin list.js was used which had to be modified to not break the form submission.

Further work has to be done when the filtering shall be combined with sorting functionality because the list.js has its own sort logic. Ordering of DOM objects outside of list.js will be overwritten by list.js. That should be addressed in a separate issue if needed.

@wvengen
Copy link
Member

wvengen commented Sep 24, 2013

Nice! There is one more thing I see that may bother users: category names are always visible. It would be most helpful to show just those categories that have matching articles; or else hide them all if a search term is active. Does that make sense? (If it exceeds the effort you'd wanted to put in, that's ok, we can add that later too)

@JuliusR
Copy link
Contributor Author

JuliusR commented Sep 24, 2013

There is one more thing I see that may bother users: category names are always visible.

Yeah, this I did by purpose (see 6ec81ac) so that the ordering of articles makes sense (grouped by category and ordered by name).

It would be most helpful to show just those categories that have matching articles

I agree that this would be the most favorable solution. However, I do not see a simple solution that is easy to understand (concerning code readability). What I thought about is using an Adjacent Sibling Selector. I will try it in another branch first and then we can discuss about the gain and readability.

or else hide them all if a search term is active

This I disklike because the ordering could confuse users.

@JuliusR
Copy link
Contributor Author

JuliusR commented Sep 24, 2013

OK with pure CSS I did not find a way. How do you like the style in the new branch?

After all, it is some more code for a slightly better user experience but I think this is fine here because the new functionality is not entangled with the main application. This means that it would not do any harm to remove or replace it at some point.

@wvengen
Copy link
Member

wvengen commented Oct 1, 2013

Congratulations for the smart categories branch, it feels quite nice.

Regarding subclassing listjs, that would be great - I'll have a quick look to see if I can come up with something. That would also allow us to use rails-assets-listjs. If not, let's merge your new branch.

@JuliusR
Copy link
Contributor Author

JuliusR commented Oct 1, 2013

That would also allow us to use rails-assets-listjs

I did not modify the original code, so in principle it should already be possible. However, an update of the gem could break compatibility of the modifications.

I will look deeply into it if you have a suggestion.

@wvengen
Copy link
Member

wvengen commented Oct 1, 2013

Ok, this also seems to work; it's not fully polished, but I'd like to call it a day.

Please make sure you have at least revision 4.

@JuliusR
Copy link
Contributor Author

JuliusR commented Oct 3, 2013

Thanks for your superClass example. In fact, I was pointing at list.customized.js, but your suggestion also helped to reduce the code additions. Regarding local variables, I only found this hint by the original author when writing a plugin: http://jonnystromberg.com/listjs-plugins-guide. I guess it does not help for our case of writing a template engine.

What do you think about the current code now?

@wvengen
Copy link
Member

wvengen commented Oct 3, 2013

Looks ok, great! Now we can use a listjs gem, right (with a commented line in the Gemfile that on updates list.*.js may need updates too)? I've been fiddling of creating a listjs plugin for delay (and one for the headings?) but didn't get there yet. Would be nice, but certainly not a must :)
Good job man.

.input-append
= text_field_tag :article, params[:article], placeholder: t('.search_article'), class: 'search-query delayed-search resettable'
%button.add-on.btn.reset-search{:type => :button, :title => t('.reset_article_search')}
%i.icon.icon-remove
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the symbol be defined in the locales? I do not think so, but there are other examples in the locales (see e.g. ui.marks.close)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I think the bootstrap icon is customisable enough (if different
icons are needed for a language, I don't think changing the font would
be a bad thing to do).

p.s. why not adding the close mark in the reset plugin (from an option
with a sensible default); that way enabling/disabling the plugin would
just work without extra html.
p.p.s. something tiny: any reason for setting plugin options at
invocation, whhen they're already the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not adding the close mark in the reset plugin

Yeah, I thought about that. Finally I prefer the current style because it is highly customizable and it is obvious how it works. Moreover, the controls are already present while the page is still loading so that the page appearance does not alter after initialization.

something tiny: any reason for setting plugin options at invocation, whhen they're already the default?

I tested if it is possible to specify alternating options. After that, I decided to leave one option per plugin so that the purpose of the [] syntax becomes clear.

@JuliusR
Copy link
Contributor Author

JuliusR commented Oct 4, 2013

I am completely satisfied now. If you are too let us briefly decide the locale issue (see my last commitcomment) and then perform the final manual tests.

wvengen added a commit that referenced this pull request Oct 8, 2013
…ified

Search articles in group_orders form
@wvengen wvengen merged commit a32a375 into foodcoops:master Oct 8, 2013
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

Successfully merging this pull request may close these issues.

None yet

2 participants