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

Use flex for sparkline text instead of block #12628

Closed
wants to merge 4 commits into from
Closed

Use flex for sparkline text instead of block #12628

wants to merge 4 commits into from

Conversation

fdellwing
Copy link
Contributor

Fixes #12451
Closes #12466

@mattab mattab added this to the 3.4.0 milestone Mar 19, 2018
@mattab mattab added Needs Review PRs that need a code review c: Design / UI For issues that impact Matomo's user interface or the design overall. labels Mar 19, 2018
@diosmosis
Copy link
Member

Hi @fdellwing, tested your PR locally on chrome & ie10 and noticed two issues.

On chrome after the initial page load, if the text next to a sparkline is too long, it can affect the image's width, due to the padding:

12628_chrome

On IE10, the flex styles didn't really work:

12628_ie10

I played around w/ the styles locally & was able to fix both by:

  1. removing the justify-content line (so it uses flex-start instead of space-around)
  2. adding the following styles to the <div> w/ the text:
    -ms-flex: 1 1 auto;
    flex: 1;

Can you make these changes? (or fix them another way)

@fdellwing
Copy link
Contributor Author

  1. This did not work for me. I fixxed it by adding flex-shrink: 0; to the img. This should also fix the test I believe.
  2. Added these options, but have no IE10 to test.

Tested the changes with FF59, Chrome 64 and IE11.

@fdellwing
Copy link
Contributor Author

The UI tests are looking pretty good now.

It seems like the rule to get the same align as before is not picked up by the UI tests.

div.sparkline script+div {
    margin: 1px 0 0 1px;
}

This is still true, so you have to live with that or provide a really ugly fix.

diosmosis
diosmosis reviewed Mar 20, 2018
View reviewed changes
plugins/Morpheus/stylesheets/main.less Outdated
display: flex;

-ms-flex: 1 1 auto;
flex: 1;
Copy link
Member

Choose a reason for hiding this comment

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

Since this element is the flex container and not a flex child, these two styles aren't needed. Can you remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add them to the text wrapper instead?

@diosmosis
Copy link
Member

Thanks for updating the PR and testing on other browsers! Left a comment, and it still isn't working on IE10 (IE10's implementation doesn't support the new flexbox syntax & is also extremely buggy). Since you can't test on IE10, I can fix it for IE10.

The screenshot tests look good, too! Can you update the expected files w/ ./console tests:sync-ui-screenshots -r matomo-org/matomo 26479?

Again, the PR looks ok, just needs some touching up, so let me know when you're done.

@fdellwing
Copy link
Contributor Author

You can change anything you want. Will remove the lines you mentioned in your comment tommorow and update screenshots afterwards.

@diosmosis
Copy link
Member

👍, thanks @fdellwing

@fdellwing
Copy link
Contributor Author 

[08:48 root@drupal-cms piwik] > ./console tests:sync-ui-screenshots -r matomo-org/matomo 26515
                                        
  [InvalidArgumentException]                               
  There are no commands defined in the "tests" namespace.

@Findus23
Copy link
Member

@fdellwing
You need to enable development mode first:

php console development:enable

@fdellwing
Copy link
Contributor Author

Still not working.

@Findus23
Copy link
Member

Findus23 commented Mar 21, 2018 •
edited

When you just run php console it should list you all available commands.

Are you running Matomo from git or the build.zip? (Okay, probably a stupid question when you are making a pull request 😄)

@fdellwing
Copy link
Contributor Author 

[09:21 root@drupal-cms piwik] > grep 'Development' -A 1 config/config.ini.php 
[Development]
enabled = 1

[09:03 root@drupal-cms piwik] > php console 
Piwik version 3.3.0

Usage:
 command [options] [arguments]

Options:
 --help (-h)           Display this help message
 --quiet (-q)          Do not output any message
 --verbose (-v|vv|vvv) Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug
 --version (-V)        Display this application version
 --ansi                Force ANSI output
 --no-ansi             Disable ANSI output
 --no-interaction (-n) Do not ask any interactive question
 --piwik-domain        Matomo URL (protocol and domain) eg. "http://matomo.example.org"
 --xhprof              Enable profiling with XHProf

Available commands:
 help                                       Displays help for a command
 list                                       Lists commands
cache
 cache:clear                                Cleares all caches. This command can be useful for instance after updating Piwik files manually.
climulti
 climulti:request                           Parses and executes the given query. See Piwik\CliMulti. Intended only for system usage.
config
 config:set                                 Set one or more config settings in the file config/config.ini.php
core
 core:archive                               Runs the CLI archiver. It is an important tool for general maintenance and to keep Piwik very fast.
 core:clear-caches                          Cleares all caches. This command can be useful for instance after updating Piwik files manually.
 core:delete-logs-data                      Delete data from the user log tables: log_visit, log_link_visit_action, log_conversion, log_conversion_item, log_action.
 core:fix-duplicate-log-actions             Removes duplicates in the log action table and fixes references to the duplicates in related tables. NOTE: This action can take a long time to run!
 core:invalidate-report-data                Invalidate archived report data by date range, site and period.
 core:purge-old-archive-data                Purges out of date and invalid archive data from archive tables.
 core:run-scheduled-tasks                   Will run all scheduled tasks due to run at this time.
 core:update                                Triggers upgrades. Use it after Piwik core or any plugin files have been updated.
custom-piwik-js
 custom-piwik-js:update                     Update the Javascript Tracker with plugin tracker additions
customvariables
 customvariables:info                       Get info about configured custom variables
 customvariables:set-max-custom-variables   Change the number of available custom variables
database
 database:optimize-archive-tables           Runs an OPTIMIZE TABLE query on the specified archive tables.
development
 development:disable                        Enable or disable development mode. See config/global.ini.php in section [Development] for more information
 development:enable                         Enable or disable development mode. See config/global.ini.php in section [Development] for more information
 development:sync-system-test-processed     For Piwik core devs. Copies processed system tests from travis artifacts to tests/PHPUnit/System/processed
 development:test-files                     Manage test files.
diagnostics
 diagnostics:analyze-archive-table          Analyze an archive table and display human readable information about what is stored. This command can be used to diagnose issues like bloated archive tables.
 diagnostics:run                            Run diagnostics to check that Piwik is installed and runs correctly
generate
 generate:angular-component                 Generates a template for an AngularJS component
 generate:angular-directive                 Generates a template for an AngularJS directive
 generate:api                               Adds an API to an existing plugin
 generate:archiver                          Adds an Archiver to an existing plugin
 generate:command                           Adds a command to an existing plugin
 generate:controller                        Adds a Controller to an existing plugin
 generate:dimension                         Adds a new dimension to an existing plugin. This allows you to persist new values during tracking.
 generate:menu                              Adds a plugin menu class to an existing plugin
 generate:plugin                            Generates a new plugin/theme including all needed files
 generate:report                            Adds a new report to an existing plugin
 generate:scheduledtask                     Adds a tasks class to an existing plugin which allows you to specify scheduled tasks
 generate:settings                          Adds a SystemSetting, UserSetting or MeasurableSetting class to an existing plugin
 generate:test                              Adds a test to an existing plugin
 generate:theme                             Generates a new plugin/theme including all needed files
 generate:update                            Adds a new update to an existing plugin or "core"
 generate:visualizationplugin               Generates a new visualization plugin including all needed files
 generate:widget                            Adds a plugin widget class to an existing plugin
log
 log:watch                                  Outputs the last parts of the log files and follows as the log file grows. Does not work on Windows
plugin
 plugin:activate                            Activate a plugin.
 plugin:deactivate                          Deactivate a plugin.
 plugin:list                                List installed plugins.
scheduled-tasks
 scheduled-tasks:run                        Will run all scheduled tasks due to run at this time.
translations
 translations:createpull                    Updates translation files
 translations:fetch                         Fetches translations files from Transifex to /var/www/piwik/tmp/transifex
 translations:generate-intl-data            Generates Intl-data for Piwik
 translations:languagecodes                 Shows available language codes
 translations:languagenames                 Shows available language names
 translations:plugins                       Shows all plugins that have own translation files
 translations:set                           Sets new translations for a given language
 translations:update                        Updates translation files
usercountry
 usercountry:attribute                      Re-attribute existing log data (visits & conversions) with geolocated location data, using the specified or configured location provider.
visitorgenerator
 visitorgenerator:anonymize-log             Anonymizes an Apache log file by anonymizing IPs and domains. It will not replace any search terms, paths or url queries. The original file will not be altered.
 visitorgenerator:generate-annotation       Generates an annotation for the current day. This command is intended for developers.
 visitorgenerator:generate-goals            Generates a few predefined goals for a specific site. If one of the predefined goals already exist they will not be created again. This command is intended for developers.
 visitorgenerator:generate-users            Generates many users. This command is intended for developers.
 visitorgenerator:generate-visits           Generates many visits for a given amount of days in the past. This command is intended for developers.
 visitorgenerator:generate-websites         Generates many websites. This command is intended for developers.
 visitorgenerator:shorten-log               Shortens an Apache log file by keeping only a small number of logs per day.

@Findus23
Copy link
Member

Findus23 commented Mar 21, 2018 •
edited

That's interesting. I have a few more commands:

➜  ~/tmp diff yours mine 
1c1
< Piwik version 3.3.0
---
> Piwik version 3.3.1-b1
45a46
>  development:sync-ui-test-screenshots       For Piwik core devs. Copies screenshots from travis artifacts to the tests/UI/expected-screenshots/ folder
64a66
>  generate:travis-yml                        Generates a .travis.yml file for a plugin. The file can be auto-updating based on the parameters supplied.
67a70,73
> git
>  git:commit                                 Commit
>  git:pull                                   Pull Piwik repo and all submodules
>  git:push                                   Push Piwik repo and all submodules
75a82,88
> tests
>  tests:coverage                             Run all phpunit tests and generate a combined code coverage
>  tests:run                                  Run Piwik PHPUnit tests one testsuite after the other
>  tests:run-aws                              Run a specific testsuite on AWS
>  tests:run-ui                               Run screenshot tests
>  tests:setup-fixture                        Create a database and fill it with data using a Piwik test fixture.
>  tests:sync-ui-screenshots                  For Piwik core devs. Copies screenshots from travis artifacts to the tests/UI/expected-screenshots/ folder
78c91
<  translations:fetch                         Fetches translations files from Transifex to /var/www/piwik/tmp/transifex
---
>  translations:fetch                         Fetches translations files from Transifex to /home/lukas/public_html/piwik/tmp/transifex
94a108
>

Just to be really sure:
You have a tests/folder?

@fdellwing
Copy link
Contributor Author

Ok, I think I have found the problem. It seems that these commands are only avaible if you pull matomo from git and run the composer install. Will look into this.

@fdellwing
Copy link
Contributor Author

Got it. This PR should be done from my side. @diosmosis it's your turn now.

@diosmosis diosmosis mentioned this pull request Mar 22, 2018
Merged
@diosmosis
Copy link
Member

Recreated in #12633 after git LFS issues w/ forks, @mattab if you verify #12633 works on IE10, it's good to merge

@mattab
Copy link
Member

mattab commented Mar 22, 2018

Thanks @fdellwing for the PR! see #12633

@mattab mattab closed this Mar 22, 2018
mattab pushed a commit that referenced this pull request Mar 22, 2018
@diosmosis @mattab
Recreating #12628: "Use flex for sparkline text instead of block" - @…
4ed366a 
…fdellwing (#12633)

* repush #12466

* dont allow the sparkline to shrink

* remove unneeded css

* Add -ms-flex to sparkline trigger correct display in IE10.

* update screenshots
@fdellwing fdellwing deleted the patch-3 branch March 28, 2018 07:03
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
@diosmosis @InfinityVoid
Recreating matomo-org#12628: "Use flex for sparkline text instead of …
028d6d4 
…block" - @fdellwing (matomo-org#12633)

* repush matomo-org#12466

* dont allow the sparkline to shrink

* remove unneeded css

* Add -ms-flex to sparkline trigger correct display in IE10.

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

@diosmosis diosmosis diosmosis left review comments

Assignees
No one assigned
Labels
c: Design / UI For issues that impact Matomo's user interface or the design overall. Needs Review PRs that need a code review
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fdellwing @diosmosis @Findus23 @mattab