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

[UX] Add configuration options to search block #2923

Closed
findlabnet opened this Issue Dec 19, 2017 · 54 comments

Comments

Projects
None yet
9 participants
@findlabnet
Copy link

findlabnet commented Dec 19, 2017

Currently search field haven't placeholder, I think it should be useful for UX.
So, core/search.module line 984 should be
'#attributes' => array('title' => t('Enter the terms you wish to search for.'), 'placeholder' => t('Search on site')),

Search form options
screen shot 2019-01-09 at 4 12 20 pm


PR by @malfroyegon: backdrop/backdrop#2051
PR by @findlabnet: backdrop/backdrop#2064
PR by @jenlampton at backdrop/backdrop#2071
New PR by @jenlampton backdrop/backdrop#2468

@malfroyegon

This comment has been minimized.

Copy link

malfroyegon commented Dec 29, 2017

Done :)

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 29, 2017

Thanks for the PR, @malfroyegon! I've just tested it on the sandbox site, works good.

Regarding the initial request, I like the option to display a placeholder for the search form but I wouldn't be happy to use it on every site, so I'd prefer a respective setting on the search form block.

@quicksketch quicksketch changed the title Add placeholder to search field [UX] Add placeholder to search field Dec 30, 2017

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Dec 30, 2017

I think adding the placeholder text is a good improvement. Making it an option might be a good enhancement for the future, but the simple addition of a hard-coded string seems like an easy starting point.

I like the string itself as well, simple and descriptive.

(p.s. Welcome @malfroyegon!)

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 30, 2017

I almost always add placeholder text to my search forms, but usually just the string 'Search'. "Search on site" seems like a funny use of language to me, but still an improvement!

@opi

This comment has been minimized.

Copy link

opi commented Dec 30, 2017

The placeholder is a short hint intended to aid the user with data entry so it should not be identical to the label element.

From https://www.w3.org/WAI/GL/low-vision-a11y-tf/wiki/Placeholder_Research#Avoid_use_of_placeholder_values

For me having a placeholder value like "Search" or "Search on site" does not describe the expected kind of value. I'd rather go with something short, like "Search terms".

@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 6, 2018

PR #2064 - simple configurable approach for this issue. Field can be just left empty.
Below addition for search settings page

placeholder

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Jan 7, 2018

Had a look at the sandbox site of the new PR. Works as expected.

In my opinion, the configuration option is important because a search block provides already a "Search" heading and a "Search" button by default. Having another "Search whatever" string as a placeholder is quite repetitive, so I'm happy to be able to not use the placeholder, thanks a lot for the PR!

That said, does it make more sense (and is it easy enough to implement) to provide the placeholder setting on the block settings form? Could be useful when placing the search form in different layouts, for instance on the home page without heading but with placeholder, and on other pages viceversa.

@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 7, 2018

@olafgrabienski, thank you for your opinion. My use case - calling search form from template programmatically and styling it like on screenshot below (no heading, no button text).

search

Maybe someone else can add something?
Anyway, apply patches for each core update - not a thing I love to do, so we need some solution.

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Jan 11, 2018

@findlabnet Thanks for adding your use case!

If nobody's up at the moment to implement a placeholder setting on the block settings form, I'd be happy to be able to use your configurable approach instead. (Guess we need some feedback on the PR from a coder perspective.)

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 11, 2018

Anyway, apply patches for each core update - not a thing I love to do, so we need some solution.

@findlabnet it's the best way to make sure those issues get fixed in core (and contrib, too!) and it's not painful if you document everything. See http://www.jenlampton.com/blog/sustainable-development-workflow-patching for how I manage it :)

...but for the current placeholder issue, this is also something that can be handled in theme, or a custom module if you need it now and don't want to worry about a core patch.

@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 12, 2018

@jenlampton, thanks for useful link. I thought that we are different from a very bureaucratic D-world and can do useful things much faster.

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jan 14, 2018

I feel like a single option for Placeholder text seems out of place when no other aspects of the form are configurable. Take @findlabnet's excellent example from the screenshot above. I think that's a typical configuration of the search form: No label (or a hidden label), no button (but usually a search icon), and a placeholder. In my experience that's actually the most common display of a search form. Anecdotally, here on GitHub, that's exactly what they do.

My feeling is that if we make placeholder configurable, so should the button text and the label (primarily so that they could be disabled).

Like @olafgrabienski, I think the most expected place for these options would be on the search block form, not the global configuration.

My use case - calling search form from template programmatically and styling it like on screenshot below (no heading, no button text).

If the search form is called from via the API, there is already the ability to modify any aspect, including the placeholder, right? What's the need for an option at all in this use-case?

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Jan 14, 2018

My feeling is that if we make placeholder configurable, so should the button text and the label.

That would be very welcome! Do you suggest to open another issue for the other elements or does it make more sense to rename this one from "Add placeholder to search field" to something like "Add configuration options to search block"?

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 15, 2018

rename this one from "Add placeholder to search field" to something like "Add configuration options to search block"?

Done, and PR filed! backdrop/backdrop#2071

screen shot 2018-01-15 at 4 52 50 pm

@jenlampton jenlampton added this to the 1.9.0 milestone Jan 15, 2018

@jenlampton jenlampton changed the title [UX] Add placeholder to search field [UX] Add configuration options to search block Jan 16, 2018

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jan 16, 2018

Nice, I really like the direction here. I also think newly placed search blocks should default to the block title being disabled. Search forms are usually clear about their purpose by their appearance, and there's no need to include a block title in most cases.

Despite this being a neat feature, it's too late for 1.9.0. We'll need more feedback and tests added to validate these new block settings.

@quicksketch quicksketch modified the milestones: 1.9.0, 1.10.0 Jan 16, 2018

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Apr 29, 2018

Changing from NR to NW because we need a checkbox for "Show submit button" instead of asking the user to leave that field blank to disable.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

This is what I was thinking initially, but we also need to account for the use-case of having a customized label that's hidden.

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jan 12, 2019

Ok, so that means no states, but the description can still go.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

fair enough, removed :) backdrop/backdrop#2468

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jan 12, 2019

Little feedback left.
I'm also thinking we should hide the block title by default. Its a bit overloaded now: Title: Search; Label: Search; Button text: Search.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

Little feedback left.

Changes made, pr updated.

I'm also thinking we should hide the block title by default.

I completely agree, but I don't think we have a clean way to change a (layout) block setting from within the (search) block configure form. Maybe with a form alter? Trying that...

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

Maybe with a form alter? Trying that...

Well, that get's it done. But it feels like a bit of a hack. Maybe Layout module should add a #block, #delta, and a '#new` for a cleaner form-alters?

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

Okay... I knew they had to be there. Helps if I look in the right place :) This feels much better.

/**
 * Implements hook_form_FORM_ID_alter().
 */
function search_form_layout_block_configure_form_alter(&$form, &$form_state) {
  // Only alter the search block's configure form.
  if ($form_state['block']->module == 'search' && $form_state['block']->delta == 'form') {
    // Only hide the block title when the search block is first added.
    if ($form_state['block']->is_new == TRUE) {
      $form['title_display']['title_display']['#default_value'] = 'none';
    }
  }
}
@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 12, 2019

More than year ago we can had two or three simple working, but not so perfect solutions.
In the meantime, we have nothing yet but the path to perfection.

Maybe we can release things by more iterative way, something better only after something working?

This is more common question, not only for this particular issue, we can all see more examples for very long approaches to achieve very simple things.

Any opinions about acceptability such workflow duration?

@docwilmot

This comment has been minimized.

Copy link
Contributor

docwilmot commented Jan 12, 2019

@findlabnet I agree its annoying when things are slow. I don't think it needs to be perfect but it still needs to be acceptable, and I think every discussion in this issue has been very reasonable thoughts and concerns. We are close now though.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

@findlabnet Thanks for the reminder.

I think on lots of issues we are pretty good about getting it done first, and then making it better later, but we do constantly have to fight the urge for perfection, and reminders help! You're right, this solution has been lagging for quite a while, but @docwilmot is also right that we're really close.

If you want to help get this done, all it needs is a review! Please log in to the sandbox, and add a search block to the site. If you like where this solution is right now, you can change the label on this issue from "pr - needs review" to "pr - reviewed and tested by the community" and that will increase the likelihood of getting this into 1.12!

@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 12, 2019

@jenlampton I glad to help, when I can. See my comment to PR.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

Thanks for that @findlabnet 👍 I have pushed a fix to the PR and it is ready for testing again. :)

@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 12, 2019

See my comment to PR. If it's OK, status can be changed to "pr - reviewed and tested by the community".

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Jan 12, 2019

See my comment to PR

Another good one. I have pushed one more (last?) fix to the PR and it is ready for (final?) testing. :)

@findlabnet

This comment has been minimized.

Copy link

findlabnet commented Jan 13, 2019

Seems good, thanks to all involved.

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Jan 14, 2019

I've merged backdrop/backdrop#2468 into 1.x for 1.12.

Thanks @findlabnet, @malfroyegon, @jenlampton, @olafgrabienski, @stpaultim, @docwilmot, and @opi!

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