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/12769] Font-Awesome Integration & Icon overhaul #3851

Merged
merged 48 commits into from Sep 20, 2015

Conversation

Projects
None yet
8 participants
@hanakin
Member

hanakin commented Aug 21, 2015

TODO:

  • Add CDN version of Font-Awesome
  • Search Icons
  • Navbar icons
  • Re-fator Drop-Downs
  • Re-factor Buttons
  • Form Icons
  • Post Icons
  • Misc Icons
  • Cleanup the semantics in the HTML
  • Cleanup /Remove unnecessary CSS
  • Remove replaced images
  • Refactor any javascript breaks
  • Cross browser checks
  • Finalize icon choices
  • Fix rtl
  • Fix responsive

Old PR: #2653
Ticket: https://tracker.phpbb.com/browse/PHPBB3-12769

Requirments: #3837

PHPBB3-12769

@marc1706 marc1706 added the 3.2 (Rhea) label Aug 22, 2015

@marc1706 marc1706 added this to the 3.2.0-a1 milestone Aug 22, 2015

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 29, 2015

Member

This is pretty much ready for review other than one small js fix for switching subscribe icons. I am going to ask for as much help with the review and cross-browser checks on this as possible as there is alot of page changes here. Not to mention alot of icons and I might have missed one

Member

hanakin commented Aug 29, 2015

This is pretty much ready for review other than one small js fix for switching subscribe icons. I am going to ask for as much help with the review and cross-browser checks on this as possible as there is alot of page changes here. Not to mention alot of icons and I might have missed one

@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/search_results.html Outdated
@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 29, 2015

Member

Nice work! My main concern is the backwards compatibility issues this will introduce for extensions. Mainly in the form of where you are deleting classes used by extensions. For example, icons in the navbar.

Is it possible we can have a small compatibility layer in the CSS where some of the (deprecated) classes you eliminated are still retained so some extensions don't get too clobbered between 3.1 and 3.2? At least for .small-icon, and if anybody can think of any others that have been important to extensions.

Member

VSEphpbb commented Aug 29, 2015

Nice work! My main concern is the backwards compatibility issues this will introduce for extensions. Mainly in the form of where you are deleting classes used by extensions. For example, icons in the navbar.

Is it possible we can have a small compatibility layer in the CSS where some of the (deprecated) classes you eliminated are still retained so some extensions don't get too clobbered between 3.1 and 3.2? At least for .small-icon, and if anybody can think of any others that have been important to extensions.

@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/navbar_header.html Outdated
@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/mcp_topic.html Outdated
@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/posting_review.html Outdated
@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/posting_review.html Outdated
@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/posting_topic_review.html Outdated
@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/ucp_pm_viewmessage.html Outdated
@VSEphpbb

View changes

Show outdated Hide outdated phpBB/styles/prosilver/template/viewtopic_body.html Outdated
@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 29, 2015

Member

@VSEphpbb there is going to be a lot of backwards compatibility issues I am sure I hade to completely rewrite several things ie buttons, action-bars, serach, drop-downs to mention a few to get this to work properly so not sure small-icon is really as big an issue by comparisons.

Member

hanakin commented Aug 29, 2015

@VSEphpbb there is going to be a lot of backwards compatibility issues I am sure I hade to completely rewrite several things ie buttons, action-bars, serach, drop-downs to mention a few to get this to work properly so not sure small-icon is really as big an issue by comparisons.

@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 29, 2015

Member

@hanakin I know .small-icon will be an issue. As for phpBB's core, we have always been trying to include as much b.c. as possible so the transition from 3.1. to 3.2 will be as smooth as possible for users, mostly with respect to extensions. Obviously there are some major changes here that may break a couple extensions in a way we can't help or forsee, but for a simple class as .small-icon and as many extensions out there that may be using (including two flagship official extension, Pages and Board Rules) I see no reason why we can't keep the old class' CSS around during the 3.2.x line.

Member

VSEphpbb commented Aug 29, 2015

@hanakin I know .small-icon will be an issue. As for phpBB's core, we have always been trying to include as much b.c. as possible so the transition from 3.1. to 3.2 will be as smooth as possible for users, mostly with respect to extensions. Obviously there are some major changes here that may break a couple extensions in a way we can't help or forsee, but for a simple class as .small-icon and as many extensions out there that may be using (including two flagship official extension, Pages and Board Rules) I see no reason why we can't keep the old class' CSS around during the 3.2.x line.

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 30, 2015

Member

@VSEphpbb we should probably update the official extension icons to be svg to avoid pixel icons mixing but I will look into the BC stuff, but this brings up an old discussion I remember having with a few of the others about including a deprecated.css file to handle such issues like this

Member

hanakin commented Aug 30, 2015

@VSEphpbb we should probably update the official extension icons to be svg to avoid pixel icons mixing but I will look into the BC stuff, but this brings up an old discussion I remember having with a few of the others about including a deprecated.css file to handle such issues like this

@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 30, 2015

Member

@hanakin We will of course update the extensions. The issue is that it currently takes several months for an extension to get validated. The reality is it will be a long time after 3.2 is released before validated extensions can catch up. We want to mitigate issues where users who choose to upgrade and find extensions are broken, and there's no update to fix it anytime soon. We really want the 3.1 to 3.2 transition to be painless.

So yeah, a deprecated css file would be great. As I said, so far I think it only needs .small-icon, though others might think of a few other rules that would help too.

Member

VSEphpbb commented Aug 30, 2015

@hanakin We will of course update the extensions. The issue is that it currently takes several months for an extension to get validated. The reality is it will be a long time after 3.2 is released before validated extensions can catch up. We want to mitigate issues where users who choose to upgrade and find extensions are broken, and there's no update to fix it anytime soon. We really want the 3.1 to 3.2 transition to be painless.

So yeah, a deprecated css file would be great. As I said, so far I think it only needs .small-icon, though others might think of a few other rules that would help too.

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 30, 2015

Member

As for now I just added the deprecated class to the end of the original files with a comment as this way you do not have to worry as much about specification with in its own file.

Member

hanakin commented Aug 30, 2015

As for now I just added the deprecated class to the end of the original files with a comment as this way you do not have to worry as much about specification with in its own file.

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 30, 2015

Member

Short of any issues anyone has with choices for the icons this is good to go as far as Browser compatability.

It has one error that has to do with the method of inclusion of Font-Awesome being from a CDN. As for some reason there is a test to ensure all styles are coming from the theme folder which obviously fails? Need to determine how we want to handle this.

@VSEphpbb if there are any more BC stuff its probably easier to fix those in alpha/beta when users can put it through the ringers. We can add BC back in at that point.

@cyberalien when you get a chance this is a large one!

Member

hanakin commented Aug 30, 2015

Short of any issues anyone has with choices for the icons this is good to go as far as Browser compatability.

It has one error that has to do with the method of inclusion of Font-Awesome being from a CDN. As for some reason there is a test to ensure all styles are coming from the theme folder which obviously fails? Need to determine how we want to handle this.

@VSEphpbb if there are any more BC stuff its probably easier to fix those in alpha/beta when users can put it through the ringers. We can add BC back in at that point.

@cyberalien when you get a chance this is a large one!

@phpbb-user phpbb-user removed the WIP 🚧 label Aug 30, 2015

@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 30, 2015

Member

@hanakin I like all the icons I've seen so far except for New Post/PM Buttons. One of the fa plus symbols makes more sense than a star to me. I know we use star currently, but we don't need to keep with it IMO. Not sure how others feel.

Member

VSEphpbb commented Aug 30, 2015

@hanakin I like all the icons I've seen so far except for New Post/PM Buttons. One of the fa plus symbols makes more sense than a star to me. I know we use star currently, but we don't need to keep with it IMO. Not sure how others feel.

@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 30, 2015

Member

I just noticed the close button icon in Ajax dialogs needs help with positioning now. It's flush up against the sides in the corner, could use some padding/margin

Member

VSEphpbb commented Aug 30, 2015

I just noticed the close button icon in Ajax dialogs needs help with positioning now. It's flush up against the sides in the corner, could use some padding/margin

@cyberalien

This comment has been minimized.

Show comment
Hide comment
@cyberalien

cyberalien Aug 30, 2015

Contributor

In navigation icon color doesn't match links. Its too saturated. Maybe something like this?

.navbar a .icon {
    color: inherit;
    opacity: 0.9;
}

Code above breaks .icon.icon-gray color, so that rule will have to be duplicated with .navbar as well.

Orange color for last post link in topics list doesn't match layout. All icons are red, orange link near red icon looks rather bad. It is also way too small, but making it larger looks equally bad. Maybe use fa-file-o instead of fa-file for it?

Similar issue with post icon inside posts (one below post subject on left of post time). It is very small.

Everything else looks good.

Contributor

cyberalien commented Aug 30, 2015

In navigation icon color doesn't match links. Its too saturated. Maybe something like this?

.navbar a .icon {
    color: inherit;
    opacity: 0.9;
}

Code above breaks .icon.icon-gray color, so that rule will have to be duplicated with .navbar as well.

Orange color for last post link in topics list doesn't match layout. All icons are red, orange link near red icon looks rather bad. It is also way too small, but making it larger looks equally bad. Maybe use fa-file-o instead of fa-file for it?

Similar issue with post icon inside posts (one below post subject on left of post time). It is very small.

Everything else looks good.

@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 30, 2015

Member

Didn't dig into find the issue, so here's a screenshot of a button I see that is messed up now by these changes:
screen shot 2015-08-30 at 2 06 43 pm

Member

VSEphpbb commented Aug 30, 2015

Didn't dig into find the issue, so here's a screenshot of a button I see that is messed up now by these changes:
screen shot 2015-08-30 at 2 06 43 pm

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 31, 2015

Member

@VSEphpbb

  • I prefer plus unless anyone else has issues.
  • The close button is there for a reason icon has no background and X is transparent if I add white in the background of it there is a slight 1px uneven ring around the icon so I opted to reposition it inside the container. Which ever way is prefered.
    screenshot 2015-08-31 16 03 49

@cyberalien

  • The icon color is slightly different because that was the color of the original icons. I can make them the same as the link if prefered.
  • icon sizes were chosen to match original icon sizes can adjust based on input I actually created three modifier classes for this reason.
  • Can you be more specific about orange icon screen shot as everything used is original colors.
Member

hanakin commented Aug 31, 2015

@VSEphpbb

  • I prefer plus unless anyone else has issues.
  • The close button is there for a reason icon has no background and X is transparent if I add white in the background of it there is a slight 1px uneven ring around the icon so I opted to reposition it inside the container. Which ever way is prefered.
    screenshot 2015-08-31 16 03 49

@cyberalien

  • The icon color is slightly different because that was the color of the original icons. I can make them the same as the link if prefered.
  • icon sizes were chosen to match original icon sizes can adjust based on input I actually created three modifier classes for this reason.
  • Can you be more specific about orange icon screen shot as everything used is original colors.
@VSEphpbb

This comment has been minimized.

Show comment
Hide comment
@VSEphpbb

VSEphpbb Aug 31, 2015

Member

@hanakin That button issue is not browser related. It's just using the button2 class which you have removed, so it lost its formatting.

Edit; Oh, it looks like you've addressed it already

Member

VSEphpbb commented Aug 31, 2015

@hanakin That button issue is not browser related. It's just using the button2 class which you have removed, so it lost its formatting.

Edit; Oh, it looks like you've addressed it already

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 31, 2015

Member

its fixed now

On Mon, Aug 31, 2015 at 8:07 AM, Matt Friedman notifications@github.com
wrote:

@hanakin https://github.com/hanakin That button issue is not browser
related. It's just using the button2 class which you have removed, so it
lost its formatting.


Reply to this email directly or view it on GitHub
#3851 (comment).

Member

hanakin commented Aug 31, 2015

its fixed now

On Mon, Aug 31, 2015 at 8:07 AM, Matt Friedman notifications@github.com
wrote:

@hanakin https://github.com/hanakin That button issue is not browser
related. It's just using the button2 class which you have removed, so it
lost its formatting.


Reply to this email directly or view it on GitHub
#3851 (comment).

@cyberalien

This comment has been minimized.

Show comment
Hide comment
@cyberalien

cyberalien Aug 31, 2015

Contributor

@hanakin

Orange icon:
icon

Small icon in post:
icon2

I've tested all possible combinations of colors and FA glyphs for those icons, don't like any of them. Maybe keep these small icons as images instead of glyphs? Update images to have HD versions. Images look much better. FontAwesome file icons are too ugly at small font size.

Contributor

cyberalien commented Aug 31, 2015

@hanakin

Orange icon:
icon

Small icon in post:
icon2

I've tested all possible combinations of colors and FA glyphs for those icons, don't like any of them. Maybe keep these small icons as images instead of glyphs? Update images to have HD versions. Images look much better. FontAwesome file icons are too ugly at small font size.

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Aug 31, 2015

Member

i do not really see the problem the icon is not that different than the
originals.

On Mon, Aug 31, 2015 at 11:47 AM, Vjacheslav Trushkin <
notifications@github.com> wrote:

@hanakin https://github.com/hanakin

Orange icon:
[image: icon]
https://cloud.githubusercontent.com/assets/822287/9576169/1adbf824-4fde-11e5-9707-1c758461f7e4.png

Small icon in post:
[image: icon2]
https://cloud.githubusercontent.com/assets/822287/9576178/35c0a4f0-4fde-11e5-9e47-282fea64cf35.png

I've tested all possible combinations of colors and FA glyphs for those
icons, don't like any of them. Maybe keep these small icons as images
instead of glyphs? Update images to have HD versions. Images look much
better. FontAwesome file icons are too ugly at small font size.


Reply to this email directly or view it on GitHub
#3851 (comment).

Member

hanakin commented Aug 31, 2015

i do not really see the problem the icon is not that different than the
originals.

On Mon, Aug 31, 2015 at 11:47 AM, Vjacheslav Trushkin <
notifications@github.com> wrote:

@hanakin https://github.com/hanakin

Orange icon:
[image: icon]
https://cloud.githubusercontent.com/assets/822287/9576169/1adbf824-4fde-11e5-9707-1c758461f7e4.png

Small icon in post:
[image: icon2]
https://cloud.githubusercontent.com/assets/822287/9576178/35c0a4f0-4fde-11e5-9e47-282fea64cf35.png

I've tested all possible combinations of colors and FA glyphs for those
icons, don't like any of them. Maybe keep these small icons as images
instead of glyphs? Update images to have HD versions. Images look much
better. FontAwesome file icons are too ugly at small font size.


Reply to this email directly or view it on GitHub
#3851 (comment).

@Nicofuma

This comment has been minimized.

Show comment
Hide comment
@Nicofuma

Nicofuma Sep 20, 2015

Member

I don't have these 404. @marc1706 can you check please?

Otherwise looks good

Member

Nicofuma commented Sep 20, 2015

I don't have these 404. @marc1706 can you check please?

Otherwise looks good

@Nicofuma

This comment has been minimized.

Show comment
Hide comment
@Nicofuma

Nicofuma Sep 20, 2015

Member

@cyberalien By the way, do you know/have an idea how much work it will require to have prosilver se compatible?

Member

Nicofuma commented Sep 20, 2015

@cyberalien By the way, do you know/have an idea how much work it will require to have prosilver se compatible?

@hanakin

This comment has been minimized.

Show comment
Hide comment
@hanakin

hanakin Sep 20, 2015

Member

@Nicofuma the 404 was fixed by hanakin@3eb63b9

Member

hanakin commented Sep 20, 2015

@Nicofuma the 404 was fixed by hanakin@3eb63b9

marc1706 added a commit to marc1706/phpbb that referenced this pull request Sep 20, 2015

Merge pull request phpbb#3851 from hanakin/ticket/12769
[ticket/12769] Font-Awesome Integration & Icon overhaul

@marc1706 marc1706 merged commit ff6d590 into phpbb:master Sep 20, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hanakin hanakin deleted the hanakin:ticket/12769 branch May 17, 2017

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