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

Update icons #8488

Merged
merged 27 commits into from Aug 24, 2015

Conversation

Projects
None yet
4 participants
@mnapoli
Contributor

mnapoli commented Aug 5, 2015

Fixes #7618

I have added the new font icons (TTF supported everywhere except IE8 + EOT for IE8 support). I have also replaced the usage of old icons. Sometimes instead of simply replacing the icons it made sense either to change the design a bit (e.g. all the user/site/goal/… "managers") or use standard UI elements (buttons, alerts, …). I haven't replaced all the icons used in Piwik (mostly because some are much harder), this can be achieved later in other pull requests.

And by the way the new icons look much better on retina ;) (not pixellized)

For reviewing the PR please checkout the branch: some design changes are better to test in a browser rather than in a screenshot, especially the buttons in the "managers" (user, site, goals, etc.).

Here are the diff screenshots: http://builds-artifacts.piwik.org/piwik/piwik/icons/14663/ You'll see that the square in the top-right has reappeared, and I don't remember what it is about and if it's a problem…

icons

mnapoli added some commits Aug 5, 2015

Import new icons proposed in #7618
The new icons are done using web fonts. They have been added to the UI demo page.

@mnapoli mnapoli self-assigned this Aug 5, 2015

@mnapoli mnapoli added this to the 2.15.0 milestone Aug 5, 2015

mnapoli added some commits Aug 5, 2015

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 13, 2015

Member

just FYI: The square is usually there when there is an update. Rebasing might help

Member

tsteur commented Aug 13, 2015

just FYI: The square is usually there when there is an update. Rebasing might help

@tsteur
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 13, 2015

Contributor

All those changes are deliberate :) Try it by pulling the branch locally, it's probably better to use it to see (hover, etc.).

To add more information on the red buttons, I've reused Piwik's default buttons (used everywhere else) for consistency (a consistent UI looks better and is easier to understand).

Contributor

mnapoli commented Aug 13, 2015

All those changes are deliberate :) Try it by pulling the branch locally, it's probably better to use it to see (hover, etc.).

To add more information on the red buttons, I've reused Piwik's default buttons (used everywhere else) for consistency (a consistent UI looks better and is easier to understand).

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 13, 2015

Member

Nice progress on icons @mnapoli!

Re: red buttons for adding "entities". Since Edit+Delete are not red buttons, it is better to be consistent in this case, and use the + icon with black text for "Create new %s". In general, the UI looks better with + icon + black text for these actions.

can you add back the "Edit" and "Delete" texts next to the icons?

Member

mattab commented Aug 13, 2015

Nice progress on icons @mnapoli!

Re: red buttons for adding "entities". Since Edit+Delete are not red buttons, it is better to be consistent in this case, and use the + icon with black text for "Create new %s". In general, the UI looks better with + icon + black text for these actions.

can you add back the "Edit" and "Delete" texts next to the icons?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 13, 2015

Contributor

@mattab have you tried it locally? I spent a lot of time to try and improve all those tables, removing this kind of redundancy using the new icons was one of the main change of this PR:

capture d ecran 2015-08-13 a 18 32 07

To this:

capture d ecran 2015-08-13 a 18 33 05

Using it isn't confusing at all: the table headers + hovering makes it really clear what such buttons do. And we are talking about the 2 most common buttons ever in a web app.

What I've tried with this PR is continue on the consistency effort by introducing new standard UI elements (like the flat buttons, icon buttons, …) and replace custom design with the existing and new standard UI components.

Contributor

mnapoli commented Aug 13, 2015

@mattab have you tried it locally? I spent a lot of time to try and improve all those tables, removing this kind of redundancy using the new icons was one of the main change of this PR:

capture d ecran 2015-08-13 a 18 32 07

To this:

capture d ecran 2015-08-13 a 18 33 05

Using it isn't confusing at all: the table headers + hovering makes it really clear what such buttons do. And we are talking about the 2 most common buttons ever in a web app.

What I've tried with this PR is continue on the consistency effort by introducing new standard UI elements (like the flat buttons, icon buttons, …) and replace custom design with the existing and new standard UI components.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Aug 13, 2015

Member

good to hear it was all planned :-)

I checked the branch and it looks nice. I have couple feedback:

  • It's maybe not a problem to have the red buttons for adding entities... what do others think?

  • warning icon is misplaced in: warning icon

  • when the "info" alerts are displayed several time in page, eg. in Geo location settings, let's remove the bulb icons in these cases:

    many icons

What's left regarding adding icons, ie. which icons are left to be ported to the new ones? would be awesome to finish this in 2.15.0 :-)

Member

mattab commented Aug 13, 2015

good to hear it was all planned :-)

I checked the branch and it looks nice. I have couple feedback:

  • It's maybe not a problem to have the red buttons for adding entities... what do others think?

  • warning icon is misplaced in: warning icon

  • when the "info" alerts are displayed several time in page, eg. in Geo location settings, let's remove the bulb icons in these cases:

    many icons

What's left regarding adding icons, ie. which icons are left to be ported to the new ones? would be awesome to finish this in 2.15.0 :-)

@mnapoli mnapoli removed their assignment Aug 17, 2015

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 17, 2015

Contributor

warning icon is misplaced in:

Fixed

when the "info" alerts are displayed several time in page, eg. in Geo location settings, let's remove the bulb icons in these cases:

Fixed

Contributor

mnapoli commented Aug 17, 2015

warning icon is misplaced in:

Fixed

when the "info" alerts are displayed several time in page, eg. in Geo location settings, let's remove the bulb icons in these cases:

Fixed

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 19, 2015

Member

I'm not sure if this was mentioned already but think it might look better to vertically align the icons in notifications/warnings on top and not in the middle.

I'd also prefer to not have the red buttons there.

What is otherwise left here? The text in edit/delete?

Member

tsteur commented Aug 19, 2015

I'm not sure if this was mentioned already but think it might look better to vertically align the icons in notifications/warnings on top and not in the middle.

I'd also prefer to not have the red buttons there.

What is otherwise left here? The text in edit/delete?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 19, 2015

Contributor

What is otherwise left here? The text in edit/delete?

What do you mean about edit/delete buttons?

it might look better to vertically align the icons in notifications/warnings on top and not in the middle.

edit: They are vertically aligned. It only aligns to the top in IE8 and PhantomJS.

Contributor

mnapoli commented Aug 19, 2015

What is otherwise left here? The text in edit/delete?

What do you mean about edit/delete buttons?

it might look better to vertically align the icons in notifications/warnings on top and not in the middle.

edit: They are vertically aligned. It only aligns to the top in IE8 and PhantomJS.

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 19, 2015

Member

edit: They are vertically aligned. It only aligns to the top in IE8 and PhantomJS.

sweet!

What do you mean about edit/delete buttons?

Matt asked: "can you add back the "Edit" and "Delete" texts next to the icons?" so I got a bit confused what the current state on this is. From what I understood it is meant to be removed and we do not want to add them back but I'm not sure if I got it right :)

Member

tsteur commented Aug 19, 2015

edit: They are vertically aligned. It only aligns to the top in IE8 and PhantomJS.

sweet!

What do you mean about edit/delete buttons?

Matt asked: "can you add back the "Edit" and "Delete" texts next to the icons?" so I got a bit confused what the current state on this is. From what I understood it is meant to be removed and we do not want to add them back but I'm not sure if I got it right :)

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Aug 22, 2015

Member

Looks good to merge after row evolution link / button thing is cleared up.

Member

diosmosis commented Aug 22, 2015

Looks good to merge after row evolution link / button thing is cleared up.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 24, 2015

Contributor

Restored the flat buttons for adding new entities, hopefully I didn't forget one.

Contributor

mnapoli commented Aug 24, 2015

Restored the flat buttons for adding new entities, hopefully I didn't forget one.

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Aug 24, 2015

Member

is it safe to merge? I could use the icons right now :)

Member

tsteur commented Aug 24, 2015

is it safe to merge? I could use the icons right now :)

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 24, 2015

Contributor

It's ready to be merged if changes are OK.

Contributor

mnapoli commented Aug 24, 2015

It's ready to be merged if changes are OK.

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Aug 24, 2015

Member

Is the row evolution 'pick row to compare' link still supposed to be a button? @mattab Can you clarify?

Member

diosmosis commented Aug 24, 2015

Is the row evolution 'pick row to compare' link still supposed to be a button? @mattab Can you clarify?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Aug 24, 2015

Contributor

@diosmosis I just made them flat (i.e. removed red buttons), problem should be solved.

Contributor

mnapoli commented Aug 24, 2015

@diosmosis I just made them flat (i.e. removed red buttons), problem should be solved.

diosmosis added a commit that referenced this pull request Aug 24, 2015

Merge pull request #8488 from piwik/icons
Fixes #7618, add new icons to Piwik & use where appropriate. Also includes slight re-designs to entity management tables & a new CSS API using the icons easily.

@diosmosis diosmosis merged commit 7de45e6 into master Aug 24, 2015

0 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
Scrutinizer Created
Details

@diosmosis diosmosis deleted the icons branch Aug 24, 2015

@diosmosis

This comment has been minimized.

Show comment
Hide comment
@diosmosis

diosmosis Aug 24, 2015

Member

Follow up issue created here: #8632

Member

diosmosis commented Aug 24, 2015

Follow up issue created here: #8632

diosmosis added a commit that referenced this pull request Aug 24, 2015

diosmosis added a commit that referenced this pull request Aug 24, 2015

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