Screenshots or links would have been useful to review the changes.

Link to diffs of the branch: http://builds-artifacts.piwik.org/ui-tests.7090/ (pick the latest)

I like the change to the tables, removing the alternate colors is a good move. The visitor log is also much better.

Here are a few regressions I have seen:

• The sorting arrow is floating on the left of the column header, which makes it appear like it's in another column

  • expected:

    

  • got:

    

• Pageviews percentage don't seem to show now: http://builds-artifacts.piwik.org/ui-tests.7090/13471.7/ActionsDataTable_pageview_percentages

• Icons to expand a subtable are not vertically aligned correctly with the text:

  

• In the visitor log, this text seems "justified" which looks weird. Before the time wasn't showing because it was overflowing (hidden). An alternate idea: show the time on a new line, but don't justify the text (and remove the -).

  

• In the visitor log, this isn't vertically aligned anymore:

  

• In the visitor log, the flag is floating in the middle in a weird way (not aligned with anything):

  

• Problems with translations here (is it related to this PR?)

  

• Percentages are in another column in http://builds-artifacts.piwik.org/ui-tests.7090/13471.7/ActionsDataTable_unique_pageview_percentages

  • expected:

    

  • got:

    

• Tables do not fit in widgets anymore (on the dashboard at least), see for example here: http://builds-artifacts.piwik.org/ui-tests.7090/13471.7/Dashboard_removed

  

• In the visitor log, the links are not blue anymore but they are black + underline: I'm not a huge fan, this is a matter of personal preference so no big deal, just asking if anyone prefer one or the other?

• Form labels have changed. See http://builds-artifacts.piwik.org/ui-tests.7090/13471.7/Installation_db_setup

I haven't reviewed all the screenshots yet.

Also given so many different things (unrelated) have been changed, it would have been better to open separate pull requests, that would be easier to review.

Plenty of small changes on the redesigns I've been working the last few weeks :) Would have been better to discuss that when I was working on it, or even afterwards. E.g. there are some things (e.g. width 40% for labels) that were discussed and chosen consciously, that are gone. But whatever let's move on.

I see that you have had to change a lot of border: @color-silver-l80; with border: @color-silver-l90; or new border-top: @color-silver-l90; borders. I've already argued for it but let's use the @theme-color-border to simplify those changes (and have more consistency). And if we need 2 colors of borders, let's have @theme-color-border and @theme-color-border-light (yes it will only cover maybe 90% of the cases, but that's the same for link colors, background colors, text color, etc.). Let's go towards simplicity and consistency rather than manually changing everything everywhere.

Also given so many different things (unrelated) have been changed, it would have been better to open separate pull requests, that would be easier to review.

Yes, that would have been better. We initially only planned to change the DataTables but one thing let to another... Next time it's hopefully smaller PR. It was also not really meant to review yet

Plenty of small changes on the redesigns I've been working the last few weeks :) Would have been better to discuss that when I was working on it, or even afterwards. E.g. there are some things (e.g. width 40% for labels) that were discussed and chosen consciously, that are gone. But whatever let's move on.

Might add the 40% for labels back again as we shortened the labels anyway. I don't know any of those discussions and think it's natural that we will change those kinda things all the time. I'm certain over the next few weeks we will see more changes and change things from this PR again.

I see that you have had to change a lot of border: @color-silver-l80; with border: @color-silver-l90; or new border-top: @color-silver-l90; borders.

As mentioned in a previous PR I'm not a fan of this border-theme-color variable. I was thinking about changing that color but wasn't sure where else it'll change etc. We currently have like 6 or 7 different greyish border colors and most of them make sense in different parts of the UI. If I find the time I will try to shrink it down to 3 or 4 different ones but not sure if I have the time.

FYI: Many of the screenshot things like "The sorting arrow is floating on the left of the column header, which makes it appear like it's in another column" and "expand icon not aligned" etc only appear in PhantomJS. I tested on a wide variety of browsers so far and looked good apart from IE 8 where the arrow icon is not fully visible.

Plenty of small changes on the redesigns I've been working the last few weeks :) Would have been better to discuss that when I was working on it, or even afterwards.

I think it's also that one always has to see it live in action and use it for a while and not only for an hour or so. It's always hard to discuss things that one has seen only on screenshots or only used for a while.

FYI: I had a look at introducing few more theme-color-border variables but it's not that easy to make it actually usable. Eg we'd rather need many pairs of background-color/border-color/font-color. Eg if you use the same border-color with different backgrounds it might be really hard to style it in a way that it looks really good. Eg if you want to use a border-color "light-red" with a red background you don't want to have the light-red border-color also used in several other places. Only where it also has the same red background-color.

That's why we currently have eg

@theme-color-background-base:          #fff;
@theme-color-background-tinyContrast:  #f2f2f2;
@theme-color-background-lowContrast:   @color-silver-l80;
@theme-color-background-contrast:      #5F5A60;
@theme-color-background-highContrast:  #202020;

Where eg base is the background color, lowContrast is used as border color and contrast or highContrast maybe as background color.

Hope you get what I mean. It would be nice to have such a system but lots of work.

Next time it's hopefully smaller PR. It was also not really meant to review yet

There was at least the "Needs review" label. And why was this merged? I only reviewed less than half of the screenshot and the code changes… And it seems nobody else reviewed it.

There was at least the "Needs review" label.

Matt added the label, I didn't know it was added. I asked and purpose was to get quick feedback already.

And why was this merged?

To release a new beta to get feedback from users already re things that are maybe not covered by UI tests or browser compatibility problems and re design in general etc. We can make more changes beginning next week if needed.

I noted 11 UI regressions in my comment above. All of them were checked, but 5 of them where not fixed. Please verify that they are indeed fixed before checking them, else I review for nothing. I've unchecked them to keep it up to date.

Also now that it's merged on master it would be great to update (and fix) the 200 ui tests: http://builds-artifacts.piwik.org/ui-tests.master/13496.7/ A lot of them are really weird, e.g. http://builds-artifacts.piwik.org/ui-tests.master/13496.7/Installation_db_setup Simply updating the screenshots isn't an option right now.

Edit: some other things broke, e.g. in WhiteLabel: http://builds-artifacts.piwik.org/protected/ui-tests.master.WhiteLabel/239.3/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/WhiteLabelSettings_overwritten_page.png&expected=WhiteLabelSettings_overwritten_page.png&github=WhiteLabelSettings_overwritten_page.png

If this pull request was still WIP then merging it on master is the opposite of what should have been done. If we want to put it on a beta soon, then it's good to communicate so that we can review it in priority.

Doing such extensive changes (in a single pull request btw) and expecting a beta the next day is a problem.

the goal was to release these visual changes as early as possible to get feedback (including your feedback here), ie. release early, release often. Some UI regressions are acceptable in beta (eg. installer). Early next week all regressions will be fixed and UI screenshot checked.

A lot of them are really weird

issue to track tweaks to do: #8157

Edit: some other things broke, e.g. in WhiteLabel:

good find! created follow up issue

Are you really trying to justify that we should merge WIP pull requests, skip (and ignore already done) code review and break master deliberately?

Just FYI: The Installation etc is not really broken. I think it only looks like this because of https://github.com/piwik/piwik/pull/8078/files#diff-d578ea669c49adb09b49c4fcf94fc79fR146 . I will have a look how to reset the screen width after that test.

Re your checkboxes:

Form labels have changed

That's on purpose. It was a bit hard to identify the different labels on a page and to "scan" a page. This way it should be a bit easier.

In the visitor log, the flag is floating in the middle in a weird way (not aligned with anything):

We're thinking it's okay. It's not completely not aligned with anything since some icons just cannot be seen as they are hidden or so with white background. It's kinda better than before.

In the visitor log, this text seems "justified" which looks weird. Before the time wasn't showing because it was overflowing (hidden). An alternate idea: show the time on a new line, but don't justify the text (and remove the -)

I saw this only in the segmented visitor log and should be fixed

Cannot reproduce the other 2. Did you test locally or only have a look at screenshot tests?

@mnapoli installer wasn't broken (we tested installer manually), it was a bug in UI test: https://github.com/piwik/piwik/pull/8164/files

I tried moved your other feedback to #8157