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
Use flex for sparkline text instead of block #12628
Conversation
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: On IE10, the flex styles didn't really work: I played around w/ the styles locally & was able to fix both by:
Can you make these changes? (or fix them another way) |
Tested the changes with FF59, Chrome 64 and IE11. |
The UI tests are looking pretty good now.
This is still true, so you have to live with that or provide a really ugly fix. |
display: flex; | ||
|
||
-ms-flex: 1 1 auto; | ||
flex: 1; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
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/ Again, the PR looks ok, just needs some touching up, so let me know when you're done. |
You can change anything you want. Will remove the lines you mentioned in your comment tommorow and update screenshots afterwards. |
👍, thanks @fdellwing |
|
@fdellwing
|
Still not working. |
When you just run Are you running Matomo from git or the build.zip? (Okay, probably a stupid question when you are making a pull request 😄) |
|
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: |
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. |
Got it. This PR should be done from my side. @diosmosis it's your turn now. |
Thanks @fdellwing for the PR! see #12633 |
…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
…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
Fixes #12451
Closes #12466