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

Jump to bottom

New design for the installation #7875

Merged
merged 2 commits into from May 20, 2015
Merged

New design for the installation #7875

merged 2 commits into from May 20, 2015

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented May 10, 2015

Related to #7584

You can see the redesign mockups here: #7584 (comment)

This pull request focuses on the redesign of visual components specific to the installation. This PR doesn't include:

Those redesigns are for generic Piwik components, so they will all have their own pull request so that we can review them more easily (and review the changes in other screens of the app).

In this PR I have created 2 new reusable components, based on Bootstrap again:

Please note that those components are HTML and CSS components, they don't require the use of JS (≠ the previous progress bar). I have documented these new components in the UI demo page in this PR (UI demo see: #7787) .

Rafa used Bootstrap to do the redesign, it only made sense to base the integration on it too. Again a reminder: this PR doesn't include Bootstrap in Piwik, I again just copy-pasted the Bootstrap code we needed.

I will post the link to the UI screenshots diff (when the tests are finished) so that you can get a better idea of what the new design look like.

@mnapoli mnapoli added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review labels May 10, 2015
@mnapoli mnapoli added this to the 2.14.0 milestone May 10, 2015
@tsteur
Copy link
Member

tsteur commented May 11, 2015

I know it doesn't really belong here as it is changing behavior and not design. I think it makes sense to remove the top "next" button eg here: http://builds-artifacts.piwik.org/ui-tests.redesign-installation/12403.7/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/Installation_system_check.png&expected=Installation_system_check.png&github=Installation_system_check.png

It is meant in a nice way (users can quickly skip this step) but it looks a bit weird and this way we kinda force users to scroll to the bottom where they might notice a warning or error. Otherwise they might just skip it.

Also it would be nice if we could make some more UI tweaks after this PR is merged. I find some things look not so nice. Eg this screen: http://builds-artifacts.piwik.org/ui-tests.redesign-installation/12403.7/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/Installation_db_setup.png&expected=Installation_db_setup.png&github=Installation_db_setup.png It looks quite "heavy" to me in general. Eg maybe we could remove some lines from the "list" component. Maybe we can also remove the horizontal lines in DB setup and change some gaps. There seems to be also different font sizes used maybe? It is not a problem of this PR as it was designed like this I think. Maybe we can just make a few adjustments after this PR.

Many screenshots / installation steps seem to be broken, are those a regression? It does now show a notification and before a form. http://builds-artifacts.piwik.org/ui-tests.redesign-installation/12403.7/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/Installation_superuser.png&expected=Installation_superuser.png&github=Installation_superuser.png http://builds-artifacts.piwik.org/ui-tests.redesign-installation/12403.7/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/Installation_superuser_fail.png&expected=Installation_superuser_fail.png&github=Installation_superuser_fail.png http://builds-artifacts.piwik.org/ui-tests.redesign-installation/12403.7/screenshot-diffs/singlediff.html?processed=../processed-ui-screenshots/Installation_setup_website.png&expected=Installation_setup_website.png&github=Installation_setup_website.png ...

tsteur
tsteur reviewed May 11, 2015
View reviewed changes
plugins/Morpheus/stylesheets/less/_variables.less
@@ -17,6 +17,8 @@
@theme-color-background-highContrast: #202020;
@theme-color-base-series: #ee3024;

@theme-color-border: @color-silver-l80;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theme-color-border is not really expressive. Border of what? I reckon we have many different border columns. I think it would be better to use @color-silver-l80 directly instead of @theme-color-border. If not, at least we should move it to a _colors.less and we shouldn't make it API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that we have that border and even more with all the redesigns (the list component, the tables, the forms)… it's useful to have a variable for that instead of every time trying to find the correct color again (happened to me a lot lately, it's honestly a real pain).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could comment the actual color value next to those variables so should be easy to find then? Maybe it's even possible to show in the style guide page? Although we probably would not want to show the colors from _colors.less there.

I was really confused by that variable and wouldn't know when to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was really confused by that variable and wouldn't know when to use it.

On the opposite that's why I created the variable: @color-silver-l80 is very confusing as we don't know when to use it, it doesn't represent anything except that it's grey. A border color is used for borders (tables, h1, hr, list-group, … anything that has a default border, so obviously you exclude cases where the border color is special like for alerts).

@mnapoli
Copy link
Contributor Author

mnapoli commented May 11, 2015

remove the top "next" button eg here

+1 with that, will do

Also it would be nice if we could make some more UI tweaks after this PR is merged.

Yes that's what I listed in the issue description. Forms are meant to be redesigned, have a look at Rafa's mockups.

Regarding UI tests I haven't fixed them all yet (WIP)

@mnapoli mnapoli mentioned this pull request May 12, 2015
Merged
@mnapoli
Copy link
Contributor Author

mnapoli commented May 12, 2015

I have opened a separate pull request for the system check table redesign: #7895 (this redesign applies to all default tables in Piwik, that's why it's separate)

Edit: have also opened "Buttons redesign #7896"

@mattab
Copy link
Member

mattab commented May 13, 2015

It is meant in a nice way (users can quickly skip this step) but it looks a bit weird and this way we kinda force users to scroll to the bottom where they might notice a warning or error. Otherwise they might just skip it.

To still make it easy to skip NEXT, would be nice to still display the button at the top when there is no error nor warning.

@tsteur
Copy link
Member

tsteur commented May 13, 2015

How do you know there is no error or warning? I'm certain in most cases people have to scroll to see all the results of the check. I think it would be more valuable to allow people to see possible warnings etc instead of just skipping this step. It will help them, it will help us in getting less issues, it will look better in the UI when the next button in the top is gone.

Assuming the system check fits on someones screen and everything is OK, then the next button would be visible anyway and the person can still skip as well easily.

@mattab
Copy link
Member

mattab commented May 13, 2015

How do you know there is no error or warning?

I meant when generating the system check we know whether there is any error or warning. When we know there was no error nor warning I suggested to display the Next button.

Assuming the system check fits on someones screen and everything is OK

fyi The system check is quite long and does not fit on one screen.

@tsteur
Copy link
Member

tsteur commented May 14, 2015

Sounds good!

@mnapoli mnapoli added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels May 14, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented May 19, 2015

UI tests are now OK, the top button is shown when there are no warnings and errors.

Ready for review and merge.

@mnapoli mnapoli added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 19, 2015
@mattab
Copy link
Member

mattab commented May 20, 2015

It looks good to me 👍

@mnapoli feel free to rebase and merge

mnapoli added a commit that referenced this pull request May 20, 2015
@mnapoli
Merge pull request #7875 from piwik/redesign-installation
bb477e3 
New design for the installation
@mnapoli mnapoli merged commit bb477e3 into master May 20, 2015
@mnapoli mnapoli deleted the redesign-installation branch May 20, 2015 11:26
@mnapoli mnapoli mentioned this pull request May 21, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mnapoli @tsteur @mattab