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/12821] Fix All gradients #2702

Closed
wants to merge 1,864 commits into from

Conversation

Projects
None yet
@hanakin
Copy link
Member

hanakin commented Jul 6, 2014

This is two changes

  1. Fix all the current gradients to be consistent and handled correctly
  2. Replace the remaining gradient gifs with css gradients IE: .headerbar, .forabg, .forumbg, .button1-3 , .forums, .dropdown-extended .header

TODO:

  • Chrome (Mac)
  • Firefox (Mac)
  • Safari (Mac)
  • Opera (Mac)
  • Chrome (Win)
  • Firefox (Win)
  • Opera (Win)
  • IE 8
  • IE 9
  • IE 10
  • Safari (IOS)
  • Chrome (IOS)
  • Android

Ticket: https://tracker.phpbb.com/browse/PHPBB3-12821

PHPBB3-12821

@hanakin hanakin changed the title Ticket/12821 Ticket/12821Fix All gradients Jul 6, 2014

@hanakin hanakin changed the title Ticket/12821Fix All gradients [ticket/12821] Fix All gradients Jul 6, 2014

@Sumanai

This comment has been minimized.

Copy link
Contributor

Sumanai commented Jul 6, 2014

-ms-linear-gradient only supported in IE10 Consumer Preview. Release IE10 support prefix-free version of the gradient. I believe that we need to remove these lines.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

yeah im aware those were in their already i have not removed them yet

@Sumanai

This comment has been minimized.

Copy link
Contributor

Sumanai commented Jul 6, 2014

Also it would be nice to use short form gradient, at least for a prefix-free gradient.
That is simply record:
background-image: linear-gradient (#d2e0eb, #eef5f9);

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

short forms are bad practice as they break consistency

@Sumanai

This comment has been minimized.

Copy link
Contributor

Sumanai commented Jul 6, 2014

Be consistent to the end, write

border-bottom-left-radius: 7px 7px;
border-bottom-right-radius: 7px 7px;
border-top-left-radius: 7px 7px;
border-top-right-radius: 7px 7px;
@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

where is that from? 7px 7px?

@Sumanai

This comment has been minimized.

Copy link
Contributor

Sumanai commented Jul 6, 2014

Did not you know? You can specify two numbers and make oval rounding. This is the most complete form of writing. If the number of one-then rounding will round.
You can experiment with different numbers, you can achieve interesting results.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

guess i have seen it but never messed with it

but I meant in prosilver where?

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

biggest problem seems to be IE-9s stupid handling of border-radius

@Sumanai

This comment has been minimized.

Copy link
Contributor

Sumanai commented Jul 6, 2014

but I meant in prosilver where?

.forabg {
    border-radius: 7px;
...

In accordance with the principle of "short forms are bad practice" to be replaced this short line to the proposed construction of me, lol.
This I mean that the short-record quite acceptable thing, if it is registered in the standard and supported by all browsers.

biggest problem seems to be IE-9s stupid handling of border-radius

You can use a small picture in SVG, encoded in BASE-64.
But it's better to just leave it at that.

@VSEphpbb

This comment has been minimized.

Copy link
Member

VSEphpbb commented Jul 6, 2014

Do we really need the browser-compatability comments on each and every gradient line in every single occurrence? Maybe just comment the first rule. otherwise it's just a waste of some precious bytes

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

@ Sumanai I am aware of the SWG but do not want to use conditionals, as to the border-radius... their prosilver is one giant inconsistency that needs fixed. one piece at a time

@VSEphpbb they are not required but its common practice to include them for future proffing

@Sumanai

This comment has been minimized.

Copy link
Contributor

Sumanai commented Jul 6, 2014

@hanakin , ie you maintain the replacement of one short line of four long? I'm afraid we do not understand each other. My knowledge of English is poor.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 6, 2014

@Sumanai apparently not please comment at the line in the code you are referring too

</li>
</ul>
<ul class="topiclist forums">
<div class="topics">

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

This is rather confusing

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

not really as wtf is a forabg, since its on the viewtopics page the section should be title topics

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

No it's not. This is the forum-list for the index.php page (and possibly viewforum.php if it has subforums).

forabg simply indicates that it is the wrap/background of the forums-list.

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

ah i did not catch that one i see what you mean

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

Also keep in mind that when you change class names like this, it can mess with a lot of extensions.

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

I get that, this is why I want to transition to Ids or data-atributes for references that way the classes do not affect them as much

http://csswizardry.com/2014/03/naming-ui-components-in-oocss/
https://github.com/csswizardry/CSS-Guidelines#js-hooks

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

That's a related problem. I was simply implying that a lot of ext already use classes like .forabg and .forumbg to style their templates (inheriting from proSilver).

So don't change them unless you absolutely have to.

background-image: -webkit-gradient(linear, left top, left bottom, color-stop(0%,#6aceff), color-stop(2px,#0076b1), color-stop(15%,#0076b1), color-stop(100%,#12a3eb)); /* Chrome,Safari4+ */
background-image: -webkit-linear-gradient(top, #6aceff 0%,#0076b1 2px,#0076b1 15%,#12a3eb 100%); /* Chrome10+,Safari5.1+ */
background-image: -o-linear-gradient(top, #6aceff 0%,#0076b1 2px,#0076b1 15%,#12a3eb 100%); /* Opera 11.10+ */
background-image: linear-gradient(to bottom, #6aceff 0%,#0076b1 2px,#0076b1 15%,#12a3eb 100%); /* W3C */

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

I think this isn't correct.

Shouldn't it be:
#6aceff 0%,#0076b1 2px,#12a3eb 92px,#12a3eb 100% ?

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

topics and forums is swapped im fixing that now

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

That's not really my point. The point is the positioning of the gradient steps.
changes

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

the header is not even 90px high and it does not account for all the other places the gradient is used which are also smaller than 90pxs possibly such as drafts so a percentage makes more sense or we move it from 90px to something like 25px or 30px besides this is the 100% conversion of the bg_header.gif to a gradient background: linear-gradient(to bottom, #6aceff 0%,#0076b1 1%,#037ebb 23%,#0f9be1 78%,#12a3eb 100%); /* W3C */

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

The fact that the header isn't 92px doesn't really matter all that much. The gradient isn't required to "finish" at the exact bottom for any particular element.

I'm merely pointing out the fact this change will scale the gradient to the entire height, which is new behavior.

If you take .forabg on the index for example, you can really see the difference (just like my screenshot above). I personally like the old look better because the dark color has the upper hand (providing more contrast). The same goes for gradient.gif (ul.forums) but in reverse; it looks significantly darker now.

But that is my personal preference.

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

its a simple change either way

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

It is. We just don't want this simple PR turning into another long "which looks better" discussion. So I would suggest sticking as close to the current look as possible, so people don't have anything to complain about.

We can discuss possible changes to the look in the future.

</ul>

<div class="<!-- IF not S_PRIVMSGS -->forums<!-- ELSE -->panel<!-- ENDIF -->">
<div class="forums-wrap">

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

Won't .forums-wrap inside a .panel give weird styling issues?

.forumbg {
background-color: #12A3EB;
background-image: url("./images/bg_header.gif");
.topics-wrap {

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

Merge with .header-wrap ?

@PayBas

This comment has been minimized.

Copy link
Contributor

PayBas commented Jul 7, 2014

I can't help feeling that 90% of this PR is just to accommodate IE9.

Whereas I fully agree that it's a good idea to replace all gradient images with CSS, I also think that many of these changes could be problematic for current extensions (particularly the extra wrap elements with overflow:hidden and class name changes).

If we cannot combine gradients with border-radius in IE9 (without all kinds of weird hacks), I would suggest we either dump border-radius for IE9, or bumping tweaks.css to IE9 and simply use the images we have now inside tweaks.css.

That means we will not be able to get rid of the image gradients completely, but they will only be used for IE9.

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 7, 2014

the other option is to add an IE9 conditional to disable the filter and use svgs for IE9 before the rest of the vendor stuff

<!--[if gte IE 9]>
    <style type="text/css"> .headerbar, .forabg, .forumbg {  filter: none;  }  </style>
<![endif]-->
.dropdown-extended .header, #tabs .tab > a, #minitabs .tab > a, .cp-mini,
.dropdown-extended a.mark_read, #cp-main .forabg, #cp-main .forumdb,
#cp-main .post, #cp-main .panel {
border-radius: 0;

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

Do we really want to remove every single border-radius? Half of these don't actually have the gradient problem since they don't have gradient backgrounds.

I know it is more consistent if we remove them everywhere, but this is a design choice, not out of technical necessity.
@prototech ^

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb Jul 7, 2014

Member

Or make a single class ie-no-radius here and add that class where needed.

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

Please no. That would make all the templates needlessly complex. Some buttons already have 6 classes.

Putting the burden on the IE9 users seems to me like a better solution.

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb Jul 7, 2014

Member

@prototech The unfortunate thing about this change is it will make Prosilver now look different than it is supposed to, squaring off most, if not all, of its corners for the first time in Prosilver's life, in a browser (IE9) that is still technically supported by 3.1. Fallbacks to even older IE browsers is even worse, with gradients going away too. This PR may be biting off more than it can chew by creating major inconsistency in prosilver's appearance in IE browsers still supported by phpBB 3.1, all for the sake of using CSS gradients instead of 4 little images!!

Again I file this under the - not worth it for current Prosilver, save it for a fresh new ground up future phpBB style redesign.

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 8, 2014

Author Member

How so? Old browsers? IE8 still looks the same or very close which is the oldest we support anything old would just default to solid color, which is their anyone still using anything old than IE8? I doubt many if any! And if need be we can add the image as a fallback if absolutely nessecary

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 8, 2014

Author Member

also here is the data for browser usage showing IE 9 being used less than IE8 and IE10 being used slightly more than IE9

http://gs.statcounter.com/#desktop-browser_version-ww-yearly-2012-2014

image

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb Jul 8, 2014

Member

I agree that this should be done too - just not now. When a major overhaul of Prosilver or a new style get conceived from the ground up, and decisions like using CSS for any gradients is made, the design can be made to accommodate that consistently across all browsers, which would mean no rounded borders at all in that case.

But here, Prosilver has been defined by its rounded borders for many years. We can't change that, in just one browser, because we are trying to switch from 4 images to CSS gradients. Those 4 images have worked very well all these years have not held back custom style development.

This is certainly no longer appropriate for a release in RC phase either.

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 8, 2014

Author Member

I completely understand your point, I am not saying change its design. I am saying their are other aspects that I have changed here that will not change it so define what we want so I can make this a useful PR. if we need images for the header I am fine with that. But rather than force everyone to use it, instead have it as a fallback for those browsers

This comment has been minimized.

Copy link
@VSEphpbb

VSEphpbb Jul 8, 2014

Member

Falling back to the original images in IE8/9 is fine for Prosilver. But doesn't that still make a style authors life a little bit more difficult as now they will need to address the CSS and the image fallbacks?

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 8, 2014

Author Member

its all part of the same rule no tweak we just remove the filter and add the image + I doubt many are going to keep it in their just to accomodate Legacy IE as its the most replaced aspect of a theme anyway

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 7, 2014

it looks horrible to leave any of them it makes more sense to make IE9 look like IE8

#cp-main .post, #cp-main .panel {
border-radius: 0;
}

This comment has been minimized.

Copy link
@MGaetan89

MGaetan89 Jul 7, 2014

Contributor

Why so many new lines?

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

because thats how many things have round corners

@@ -69,5 +69,22 @@ dd.option {

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

This comment has been minimized.

Copy link
@PayBas

PayBas Jul 7, 2014

Contributor

This is a typo? Or some hax I don't know about.

This comment has been minimized.

Copy link
@hanakin

hanakin Jul 7, 2014

Author Member

yeah should be \9

@nickvergessen

This comment has been minimized.

Copy link
Contributor

nickvergessen commented Jul 27, 2014

We use uppercase for colors, would be nice if you replace them (e.g. #FFFFFF instead of #ffffff )

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Jul 28, 2014

Fixed the colors to be uppercase, and I only currently have a local test environment. I can set one up though on my server tonight

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 1, 2014

@nickvergessen ok so I finally managed to get a test server up and running http://12821.midaym.com

username: user1
password: password

Nicofuma and others added some commits May 29, 2015

Merge pull request #3616 from s9e/ticket/13847
[ticket/13847] Move quote generation to text_formatter.utils
Merge pull request #3613 from nickvergessen/ticket/13844
Ticket/13844 Better FAQ language files
Merge pull request #3383 from nickvergessen/ticket/9109
Ticket/9109 Properly document and calculate the group settings with value 0
Merge pull request #3561 from s9e/ticket/10922
[ticket/10922] Added support for body and subject in email BBCode
Merge pull request #3660 from Nicofuma/ticket/13890
[ticket/13890] Fix di tests
[ticket/13388] Fix rebase
PHPBB3-13388
Merge pull request #3639 from marc1706/ticket/13875
[ticket/13875] Ignore cache, ext, and store folder in lint test

* marc1706/ticket/13875:
  [ticket/13875] Ignore cache, ext, and store folder in lint test
@Nicofuma

This comment has been minimized.

Copy link
Member

Nicofuma commented May 29, 2015

bump @hanakin
rebase on 3.2.x please and send a new against this branch

bantu and others added some commits May 29, 2015

Merge branch '3.0.x' into 3.1.x
* 3.0.x:
  [ticket/13875] Ignore cache, ext, and store folder in lint test

Conflicts:
	tests/lint_test.php
Merge branch '3.1.x'
* 3.1.x:
  [ticket/13875] Ignore cache, ext, and store folder in lint test
Merge pull request #3560 from Nicofuma/ticket/13790
[ticket/13790] Update phpcs
Merge branch '3.1.x'
* 3.1.x:
  [ticket/13790] Update phpcs

Conflicts:
	phpBB/composer.json
	phpBB/composer.lock
Merge pull request #3545 from VSEphpbb/ticket/13771
[ticket/13771] Allow AJAX errors to support exceptions messages
Merge branch '3.1.x'
Conflicts:
	phpBB/styles/subsilver2/template/posting_body.html
Merge pull request #3638 from naderman/ticket/13874
[ticket/13874] Add master to sami API docs
Merge pull request #3592 from Nicofuma/ticket/13388
[ticket/13388] Integrate routing and di parameters resolution

@hanakin hanakin closed this May 30, 2015

@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented May 30, 2015

Its probably better to split this up into pieces anyway

@hanakin hanakin referenced this pull request Aug 25, 2015

Merged

[ticket/12821] Convert all images to gradients #3861

4 of 4 tasks complete
@hanakin

This comment has been minimized.

Copy link
Member Author

hanakin commented Aug 25, 2015

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.