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

List view directory search #32

Merged
merged 6 commits into from
Mar 1, 2018

Conversation

proxyconcept
Copy link

Can resolve #16 :

  • The new config option $results_display_mode can be "boxes" (default) or "table".
  • Two sub-templates are added : listing_boxes.tpl and listing_table.tpl, so the latter is shared with the directory view.
  • When displaying search results with "table", columns are made from $search_result_title and $search_result_items.

Can partially resolve #1 :

  • When displaying search results with "table", the Datatables JS component can be used for pagination
  • New config option $use_datatables to enable/disable the Datatables component in the application
  • New config option $datatables_page_length_choices (optional) to set the list of available page lengths
  • New config option $datatables_page_length_default (previously named $default_page_length) to set the default page length for Datatables (can also be used to force the length without $databels_page_length_choices)
  • But Add pagination to results #1 can still stay opened because we can imagine another pagination system when search results are displayed in boxes

And as mentioned in #28 :

  • Specific CSS style for links "mailto:" and "tel:"
  • Config option $directory_linkto can also be a boolean value (instead of a list of columns) : true if you want the whole row clickable ; false do the same and hide the button in the first column.
  • New config option $search_result_linkto : same as $directory_linkto for the search results in table.

Other new config options :

  • $directory_show_undefined : same as $search_result_show_undefined for the display view.
  • $display_show_undefined : same as $search_result_show_undefined for the display view.

Sorry for the big commits... It was hard to keep it smaller. I also made changes on smarty assignments in order to not load useless data and improve the factorization.

@coudot coudot self-requested a review February 27, 2018 07:24
@coudot
Copy link
Member

coudot commented Feb 27, 2018

Thanks, I will need some time to review all the changes. I will try to do it as soon as possible.

@nqb
Copy link
Contributor

nqb commented Feb 27, 2018

Hello @proxyconcept,

When displaying search results with "table", columns are made from $search_result_title and $search_result_items.

Do you use $search_result_title to keep compatibility with display view ? FYI, this variable is not use in directory view. It could be more consistent to have same display between two views.

Specific CSS style for links "mailto:" and "tel:"

I notice that cursor is a cross when you move it on a mailto: or tel: link:
Expected behavior ?

$directory_show_undefined : same as $search_result_show_undefined for the display view.

I suppose you would like to mean "directory view".

@nqb
Copy link
Contributor

nqb commented Feb 27, 2018

New config option $datatables_page_length_choices (optional) to set the list of available page lengths

If I set :

$datatables_page_length_choices = array(10, 25, 50, 100, 250, 500, -1);

I can't see 250 and 500 values in dropdown.

@proxyconcept
Copy link
Author

Do you use $search_result_title to keep compatibility with display view ?

I use $search_result_title and $search_result_items because they are used to construct the LDAP request. If we use another config options :

  • $search_result_title and $search_result_items would become useless with $results_display_mode = 'table'
  • It may be interesting to have different presentation between search results and directory view
  • We can't use the config option $directory_items for search results display : it would be inconsistent especially when you don't use the directory view

So, I think we can :

  • decide that the display is in all cases common between directory and results in table, then use a unique config option with an appropriate name
  • keep a configuration for each view, for example with a new $search_result_columns (and rename $directory_items in $directory_columns)
  • or just do with the current options... this is what I do with $search_result_title and $search_result_items

I don't say my actual solution is the best, but I can't decide on this point. So I chose the least impacting option.

I notice that cursor is a cross when you move it on a mailto: or tel: link:

Yes... just because I wanted to use a different cursor than the classic pointer. it seems to me useful when the pointer cursor indicates that the link (or the entire row) is clickable to open the display view.
But unfortunately I found nothing better than the cross cursor in the standard cursors.

I suppose you would like to mean "directory view".

Right... lazy copy/paste

If I set $datatables_page_length_choices = array(10, 25, 50, 100, 250, 500, -1); I can't see this values in dropdown.

Well... It works for me

@coudot coudot added this to the 0.2 milestone Feb 28, 2018
Copy link
Member

@coudot coudot left a comment

Choose a reason for hiding this comment

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

Hello, here are some feedback for discussion:

  • It is not clear why we have 2 print buttons (one for list, one for page) in directory page
  • If we use links on attributes in the table, I think we must remove the button a the beginning of the line
  • I am not very fond of the dotted line for tel and mail links in directory table. But it is a nice idea to add the phone and mail icon when hover the link, so we can see it is not a common link. I would also remove the cross to keep the default link cursor. Using the icon is enough from my point of view
  • I would also change the hover color of row and the print button style, but that is something I will do after merging

Please let me know when you think the PR is complete so we can merge it.

@nqb
Copy link
Contributor

nqb commented Feb 28, 2018

If I set :

$datatables_page_length_choices = array(10, 25, 50, 100, 250, 500, -1);

I can't see 250 and 500 values in dropdown.

Sorry, it was an error. Great enhancement !

@coudot
Copy link
Member

coudot commented Mar 1, 2018

I will merge this PR and do some adaptations after.

@coudot coudot merged commit 69fd811 into ltb-project:master Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add pagination to results Print results as rows or boxes
3 participants