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

API: Treedropdownfield showsearch default true, provide better ui #2364

Merged
merged 1 commit into from
Aug 29, 2013

Conversation

adrexia
Copy link
Contributor

@adrexia adrexia commented Aug 26, 2013

  • Set search option true on treedropdown fields by default, to provide a fallback solution when trees fail to render (too many children errors)
  • Provide better indication/more meaningful styling to search (match chosen styles for consistency)

Tested in Chrome, Firefox, IE8 and IE9

tree

@chillu
Copy link
Member

chillu commented Aug 28, 2013

Hey Naomi, that's a really cool solution for this, so simple and effective :) Some notes:

  • It doesn't work for me on the "insert link" panel though in Chrome, maybe something to do with z-index and focus?
  • When defocusing the expanded panel after doing a search without selecting an item, it goes back to "choose". But the filtered tree panel sticks around without indicating that its filtered. Preexisting issue, but now that we have a separate search input field: Could you change that to keep the search value in there, even when the panel is closed?
  • The loading indicator sometimes shows alongside the triangle, and is not really well centred (although that might've been a preexisting problem).
  • The whole triangle left padding looks out of place a bit now that we have the search form there.

@chillu
Copy link
Member

chillu commented Aug 28, 2013

Also, in terms of merging this into 3.1: Its kind of an API change, but not a very disruptive one. It simply makes an existing field easier to use. We should nevertheless mention it, since some TreeDropdownFields might deal with hierarchical data that needs a specialized setSearchFunction(). Can you add a docs/en/changelogs/3.1.1.md file and start an "upgrading" section?

Set search option true on treedropdown fields by default, to provide a
fallback solution when trees fail to render (too many children errors)

Provide better indication/more meaningful styling to search (match
chosen styles for consistency)
@adrexia
Copy link
Contributor Author

adrexia commented Aug 29, 2013

  1. Fixed. It was a z-index thing, but a really strange one. It worked fine in firefox, and it didn't look like z-index should have been an issue. It seems to have needed to be set quite high, so it could be that it needs to have a higher z-index that it's parent
  2. Fixed. I left the bit where it clears the field onfocus in, because I think I would want it to work that way. There is the possibility of it confusing people, but I think the pay off (easily adding new criteria) could be worth it, and it still takes interacting with the field to clear it. Thoughts?
  3. This was preexisting, but I moved the loading indicator to the right to get it out of the way. I think, ideally, it would be near the dropdown trigger in the select, but the effort involved in moving it there seems out of proportion to the benefits.
  4. Moved triangle to align with the search. I left these in the same scss that the other treedropdownfield changes are in, because they are more connected with the treedropdown than the generic tree styling. Is this ok, or should I move them into _tree.scss?

Added changelog file. First time I've ever done that though, so not sure if I did it right.

chillu added a commit that referenced this pull request Aug 29, 2013
API: Treedropdownfield showsearch default true, provide better ui
@chillu chillu merged commit 4ff7b43 into silverstripe:3.1 Aug 29, 2013
@chillu
Copy link
Member

chillu commented Aug 29, 2013

Yeehaw, we're finally getting close to an actually useable tree UI control. Thanks so much for taking that on Naomi. The upgrading guide is fine. Maybe a bit verbose (we try to keep it easy to scan and comprehend), but we can tidy that up before the release.

@ARNHOE
Copy link

ARNHOE commented Aug 29, 2013

@chillu @adrexia Hate to ring the bell when things aren't working, but currently when trying to select a parent page in Add Page. It stays with (Choose), I have checked if it only happens with custom page types but this isn't the case.

It does select the page type when adding a page with parent selected, so the URL Parameters work.

Using latest branch of 3.1 on Mac OS X with Chrome.

@chillu
Copy link
Member

chillu commented Aug 29, 2013

@adrexia You changed the semantics of getValue() to return a title instead, which means updateTitle() breaks.
Could you try to fix that, first thing when you come into the office ideally?

Also, the change to default $showSearch=true did have some unintended side effects. It reveals that the search isn't actually operational for SiteTree and File records since the $labelField defaults to "TreeTitle". That's a composite field from PHP, not an acutal DB field, which fails the search SQL. We've used setSearchCallback() in all places where search was enabled previously. That's a bit hacky, so I've added some fallback to TreeDropdownField.
Could you please review #2373 ?

@chillu
Copy link
Member

chillu commented Aug 30, 2013

I've removed placeholder fallbacks, which makes getValue() operational again: 93ea066. Reasoning in commit message. We could look into a generic placeholder polyfill. The most common one is https://github.com/mathiasbynens/jquery-placeholder, but it was year old unmerged pull requests and 50+ issues on 175 lines of code...

chillu added a commit that referenced this pull request Aug 30, 2013
It breaks the semantics of getValue(), leading to a broken field.
Regression from 8b5f89f. In the end, placeholder support is
considered "progressive enhancement", the search box should
be pretty obvious to IE8/IE9 users either way, given the main
field label is called "choose or search".
mandrew pushed a commit to mandrew/silverstripe-framework that referenced this pull request Oct 31, 2013
This is a workaround in order to ensure the field stays operational
for SiteTree and File records with the new $showSearch=true default.
Previously it was necessary to use setSearchCallback(), otherwise
the SQL query would fail. One limitation to keep this change generic
is that "MenuTitle" won't be used to search, since its SiteTree specific,
while the "Title" and "Name" fields are generally regarded as
model conventions (e.g. they're used in DataObject->getTitle() as well).

See silverstripe#2364
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

3 participants