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

[ticket/12598] Improve search box styling #2504

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
9 participants
@PayBas
Copy link
Contributor

commented May 27, 2014

https://tracker.phpbb.com/browse/PHPBB3-12598

18-9-2014 tests:

  • FF 32, normal, responsive, RTL
  • Chrome 37, normal, responsive, RTL
  • Opera 23, normal, responsive, RTL
  • Safari 5.1.7, normal, responsive, RTL (doesn't support font-size: 0)
  • IE11, normal, responsive, RTL
  • IE10, normal, responsive, RTL
  • IE9, normal, responsive, RTL
  • IE8, normal, "responsive", RTL
  • Android Chrome, responsive, RTL

newsearch
newsearch
PHPBB3-12598

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented May 27, 2014

I think this is a nice thing for 3.2? :)

</ul>
</div>
<!-- ENDIF -->
</div>

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 27, 2014

Contributor

Missing new line

{S_SEARCH_HIDDEN_FIELDS}
<!-- ENDIF -->

<input class="button" type="submit" value="{L_SEARCH}" />

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 27, 2014

Contributor

I don't think we should use button here, as it highlights it with red text.

This comment has been minimized.

Copy link
@PayBas

PayBas May 27, 2014

Author Contributor

That won't matter if we use an icon instead of text. Check the latest screenshot here:
http://area51.phpbb.com/phpBB/viewtopic.php?f=108&p=264577#p264575

background-image: url("./images/icon_textbox_search.gif"), linear-gradient(to bottom, #FFFFFF, #E9E9E9);
background-position: 50% 50%, 0 0;
background-repeat: no-repeat, repeat;
-ms-filter: "none";

This comment has been minimized.

Copy link
@PayBas

PayBas May 27, 2014

Author Contributor

Since input elements don't support :before and :after, we have to rely on using multiple backgrounds. IE8 doesn't support this. IE9 does, but doesn't support gradients (without filter).

So we would have to use a combination of filter: and background-image, but this usually either breaks IE8, or IE9, or both.

Since an icon is more important than a gradient, I removed the filter and use only the icon. IE8 has the nasty habit of trying to load multiple background-images (even though it doesn't support it) when filters are defined, so we have to define an extra rule for IE8 in tweaks.css that "resets" the background.

I'll update it if I find a better solution.

@@ -237,6 +237,10 @@ div[class].topic-actions {
height: auto;
}

.topic-actions ul.linklist li {
font-size: 1.0em;

This comment has been minimized.

Copy link
@PayBas

PayBas May 27, 2014

Author Contributor

.topic-actions and ul.linklist li both set font-size to 1.1em. The result is that the "mark forums read" text will be too big. So we scale it back to normal using this rule.


/* Fixes search-tools background in IE8 (no gradient, but at least we have an icon) */
.search-tools input.button {
background: url("./images/icon_textbox_search.gif") no-repeat 50% 50%;

This comment has been minimized.

Copy link
@PayBas

PayBas May 27, 2014

Author Contributor

See comment above

overflow: hidden;
padding: 3px;
position: relative;
text-indent: -5000px;

This comment has been minimized.

Copy link
@PayBas

PayBas May 27, 2014

Author Contributor

We hide the button text, but don't remove the value of input for the visually-impaired

@PayBas PayBas changed the title [WIP] [ticket/12598] Change predefined search-query links to search form drop-down menu [ticket/12598] Change predefined search-query links to search form drop-down menu May 27, 2014

@PayBas PayBas changed the title [ticket/12598] Change predefined search-query links to search form drop-down menu [WIP] [ticket/12598] Change predefined search-query links to search form drop-down menu May 27, 2014

@PayBas PayBas changed the title [WIP] [ticket/12598] Change predefined search-query links to search form drop-down menu [ticket/12598] Change predefined search-query links to search form drop-down menu May 28, 2014

<!-- IF S_USER_LOGGED_IN and not S_IS_BOT and U_MARK_FORUMS -->
<div class="pagination">
<a href="{U_MARK_FORUMS}" accesskey="m" data-ajax="mark_forums_read">{L_MARK_FORUMS_READ}</a></li>
</div>

This comment has been minimized.

Copy link
@PayBas

PayBas May 28, 2014

Author Contributor

I know we don't actually have pagination on the index, but the .pagination class inside of .topic-actions gives us the exact layout we need (including responsive and bidi).

If we use .linklist, the responsive JS will kick in, and create a drop-down menu for 1 single item, which is not what we want.

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb May 28, 2014

Member

can you create a new class though for this usage and copy the parts of the pagination class that give you what you want? We shouldn't use a pagination class for anything that is not pagination (what if it gets changed at a later date).

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb May 28, 2014

Member

If these are the View Unanswered posts, New Posts, Unread Posts, Active Topics links that appear at the top of the board index only, I think they should be left as is. These are incredibly commonly used links by users when they return to a forum, and should not be hidden away and no longer one-click actions.

This comment has been minimized.

Copy link
@PayBas

PayBas May 28, 2014

Author Contributor

One of the main reasons for this was in fact to hide them. They clutter the layout quite a lot and even get a separate drop-down menu in responsive mode (so they are hidden anyway). This puts them into yet another nondescript dropdown with only the default (3 stripes) menu icon.

They are search functions, and should logically be close to the other search functions.

But better yet: by including them in the search-tools, these links are accessible from almost every page, instead of only the index!

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb May 28, 2014

Member

That's all fine, I'm just saying that on the board index page only, I'd prefer it to remain as it has always been (there never was a search field there anyways). They are search functions, yes, but on the board index page, they are quick-access links.

@prototech @nickvergessen ?

This comment has been minimized.

Copy link
@PayBas

PayBas May 28, 2014

Author Contributor

So I'll just leave them in the search-tools, but remove the search tools from the index and put the old links back.

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen May 28, 2014

Contributor

Well maybe we can add a dropdown "quick-link" or something like that on the index (don't implement, but discuss this!!!!!1eleven)

This comment has been minimized.

Copy link
@hanakin

hanakin May 29, 2014

Member

I disagree with that assessment, as thats the main problem with prosilver now is that we focus on adding too much "quick access" rudimentary UI instead of actually thinking it through based on some or even any form of UI theory. Yes it will move locations and may take sometime to figure out but the benefits out-way the negative and its utilized similarly on other forums. http://esotalk.org/forum/

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb May 29, 2014

Member

As per the index page, what are the benefits of hiding these quick access links behind a Javascript-required menu? In this instance, it reduces accessibility, hides them from new users and will bother users trained to use those links.

As for the rest of the pages, this change is great! Just not on the board index, which has always been a special case where these quick access links were put in place for a reason.

This comment has been minimized.

Copy link
@PayBas

PayBas May 29, 2014

Author Contributor

I'm perfectly fine with this. I really like having these links available on (almost) every page. And of course the much better looking search-box in the topic-actions.

So like I said before: I can leave the index as-is, and only implement it for the other pages. There will be no possibility of confusion there, as nothing is removed. Everything will be in the normal place. Just with some extra functionality.

Depending on the feedback we get, we can choose to implement it differently in 3.2?

@PayBas PayBas changed the title [ticket/12598] Change predefined search-query links to search form drop-down menu [3.2][ticket/12598] Change predefined search-query links to search form drop-down menu May 29, 2014

.topic-actions:after {
content: '';
clear: both;
display: block;

This comment has been minimized.

Copy link
@PayBas

PayBas May 29, 2014

Author Contributor

Also included in: #2517

@PayBas PayBas changed the title [3.2][ticket/12598] Change predefined search-query links to search form drop-down menu [ticket/12598] Change predefined search-query links to search form drop-down menu May 30, 2014

@PayBas PayBas changed the title [ticket/12598] Change predefined search-query links to search form drop-down menu [ticket/12598] Improve action-bar search box styling and add predefined searches to drop-down menu May 30, 2014

@VSEphpbb

This comment has been minimized.

Copy link
Member

commented Jun 2, 2014

Awesome. Few things:

  1. Can the circle/x button shown in the field when there is text turn the mouse to pointer when hovered over it?
  2. Can the Search Tools Drop Down disappear when the mouse clicks inside the text field (right now only disappears when clicking anywhere else on the page, except inside the text field)?
@PayBas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2014

  1. I usually don't like messing too much with very browser-specific pseudocodes, but what you're asking could be done with: input[type="search"]::-webkit-search-cancel-button { cursor: pointer; }
    Do you want this for all search fields?

  2. Yeah that's probably a good idea

edit: both have been implemented

@PayBas PayBas changed the title [ticket/12598] Improve action-bar search box styling and add predefined searches to drop-down menu [ticket/12598] Improve action-bar search box styling Jun 3, 2014

@PayBas

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2014

Dropdown functionality has been removed

See http://area51.phpbb.com/phpBB/viewtopic.php?p=265243 or IRC logs

<input class="button2" type="submit" name="submit" value="{L_SEARCH}" />
<!-- ENDIF -->
</div>
<div class="search-box">

This comment has been minimized.

Copy link
@prototech

prototech Jun 13, 2014

Contributor

The indentation was fine as it was. The HTML should be a tab deeper than its containing <!-- IF -->.

.search-box button.search:before {
background: transparent 50% 50% no-repeat;
content: '';
display: inline-block;

This comment has been minimized.

Copy link
@prototech

prototech Jun 13, 2014

Contributor

Use .icon-button alongside .search and you should be able to get rid of the first three properties. May need a margin: 0;.

}

.search-box a.button:before {
float: left;

This comment has been minimized.

Copy link
@prototech

prototech Jun 13, 2014

Contributor

Why is this needed?

.search-box input {
.search-box .inputbox {
background-image: none;
border: 1px solid transparent;

This comment has been minimized.

Copy link
@prototech

prototech Jun 13, 2014

Contributor

.inputbox already defines this. Why is it necessary to set it again here?

font-size: 0;
height: 24px;
margin: 0;
overflow: hidden;

This comment has been minimized.

Copy link
@prototech

prototech Jun 13, 2014

Contributor

Is this necessary?

@nickvergessen nickvergessen changed the title [ticket/12598] Improve action-bar search box styling [3.2][ticket/12598] Improve action-bar search box styling Jun 16, 2014

@PayBas PayBas changed the title [3.2][ticket/12598] Improve action-bar search box styling [3.2][WIP][ticket/12598] Improve action-bar search box styling Jun 22, 2014

@PayBas PayBas changed the title [3.2][WIP][ticket/12598] Improve action-bar search box styling [3.2][ticket/12598] Improve action-bar search box styling Jun 22, 2014

@PayBas PayBas changed the title [3.2][ticket/12598] Improve action-bar search box styling [3.2][ticket/12598] Improve search box styling Jun 22, 2014

@PayBas

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2014

I don't see the problem with leaving it in. Extensions can use it if they wish.

@@ -6,16 +6,16 @@ tweaks required due to its poor CSS support.

/* Clear float fix for IE7 */
.inner {
zoom: 1;
zoom: 1\9;

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Aug 23, 2014

Contributor

Can you add a comment to the initial doc block so every one knows which browser this is for and what it does?

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 23, 2014

Member

the whole file should be fixed of this as do we even support IE7 anymore?

This comment has been minimized.

Copy link
@PayBas

PayBas Aug 23, 2014

Author Contributor

I'll add some documentation for the * and \9 hacks we currently use.

That way this PR can be merged without yet more testing delays.

We'll just make a ticket specifically for removing legacy hacks and tweaks.

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 23, 2014

Member

haha already have it the new tweaks file lol

/* Style Sheet Tweaks

These style definitions are IE 8 and 9 specific
tweaks required due to its poor CSS support.
-------------------------------------------------*/

/* 
    IE 8 Fixes 
*/

/* Clear float fix */
.inner { zoom: 1\9; }
ul.linklist { zoom: 1\9; }

/* Align checkboxes/radio buttons nicely */
dd label input { vertical-align: text-bottom\9; }

/* Fixes header-avatar aspect-ratio */
.header-avatar img { height: 20px\9; }

/* IE8 often can't handle max-width in %, so we use px instead */
.postprofile .avatar img {  max-width: 150px\9; }

/* 
    IE 9 Fixes 
*/

/* Border-radius bleed fix*/
#search-box, 
#search-box .inputbox, 
#search-box a.button { border-radius: 0; }
@@ -69,10 +69,15 @@ dd.option {

/* Fixes header-avatar aspect-ratio in IE8 */
.header-avatar img {
height: 20px;
height: 20px\9;
}

/* IE8 often can't handle max-width in %, so we use px instead */
.postprofile .avatar img {
max-width: 150px;

This comment has been minimized.

Copy link
@hanakin

hanakin Aug 23, 2014

Member

this needs a \9

@PayBas PayBas force-pushed the PayBas:ticket/12598 branch from f3aec60 to 0e60357 Aug 23, 2014

@hanakin hanakin referenced this pull request Aug 23, 2014

Merged

[Ticket/13009] Cleanup Tweaks CSS #2906

2 of 2 tasks complete
@Nicofuma

This comment has been minimized.

Copy link
Member

commented Sep 14, 2014

Rebase please and then @prototech

@PayBas PayBas force-pushed the PayBas:ticket/12598 branch from f11ad12 to e5f3137 Sep 15, 2014

@PayBas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

Rebased

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2014

While I like the new look, I'm just not sure whether we should do this so late into the game.

@Nicofuma

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

It is an important improvement. Si it's a +1 for me but it should be hardly tested.

@PayBas

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2014

I think this has been tested through and through already. But I'll do a final round of testing later today.

In any case. If any problems should surface after release... updating a style is way easier than updating the core ;).

@marc1706

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

+1 from me

}

#site-description h1 {
margin-right: 0;
}

/* Search box
--------------------------------------------- */
#search-box {

This comment has been minimized.

Copy link
@hanakin

hanakin Sep 18, 2014

Member

is their a reason this is an ID rather than a class?


/* Border-radius bleed fix in IE9 */
#search-box, #search-box .inputbox, #search-box a.button {
border-radius: 0;

This comment has been minimized.

Copy link
@hanakin

hanakin Sep 18, 2014

Member

same here why are you using the id rather than the class?

@PayBas PayBas force-pushed the PayBas:ticket/12598 branch from 3f564f0 to 6eb2b87 Sep 18, 2014

@prototech

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2014

Replaced by #2968

@prototech prototech closed this Sep 18, 2014

@PayBas PayBas deleted the PayBas:ticket/12598 branch Sep 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.