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.

Sign up for GitHub

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 #12466

Closed
wants to merge 5 commits into from
Closed

Use flex for sparkline text instead of block #12466

wants to merge 5 commits into from

Conversation

fdellwing
Copy link
Contributor

@fdellwing fdellwing commented Jan 17, 2018 •
edited by Findus23
Loading

Fixes #12451

@fdellwing
Copy link
Contributor Author

Some things don’t look as expected. Will look into that.

@fdellwing
Copy link
Contributor Author

Travis did not upload the screenshots to https://builds-artifacts.piwik.org/matomo-org/matomo/3.x-dev/25728/

Can someone trigger the build again or do I need to push something?

@Findus23
Copy link
Member

@fdellwing That's odd, I restarted the build (https://travis-ci.org/matomo-org/matomo/builds/329917019)

@fdellwing
Copy link
Contributor Author

Ok, thats a lot better. But Ecommerce is a problem, but I do not have an matomo instance where I can test this. Does anyone has an instance where I could test this widgets?

@mattab mattab added this to the 3.3.1 milestone Jan 18, 2018
@mattab
Copy link
Member

mattab commented Jan 18, 2018 •
edited
Loading

HI @fdellwing . Thanks for the PR. Here is my feedback/review:

@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Jan 18, 2018
@fdellwing
Copy link
Contributor Author

fdellwing commented Jan 18, 2018 •
edited
Loading

The flex seems to cause some of the sparklines images to shrink in width.

This is just a problem in the UI Tests. In "real" the sparklines have the same width as always, I dont know why this happens in the test. Maybe because the placeholder grafic behaves different.

The Ecommerce data can be generated using the Visitor Generator

Will look into this. But probably after the weekend as I'm quite busy at work with things I get paid for ;)

The vertical alignment has changed in the PR.

Thats a result in using flex instead of block. It is not really visible for the user, but I can fix that if wanted.

@mattab mattab modified the milestones: 3.3.1, 3.4.0 Jan 19, 2018
fixxes ecommerce flex
c3eacef 
fixxes alignment
fixxes UserCountry module
@fdellwing
Copy link
Contributor Author

Something seems to be broken with Tests, I can not really check what my changes are.

It seems to have something to do with #12463.

Can you update the screens please and rerun the build? Will look into the results on monday.

@fdellwing
Copy link
Contributor Author

@Findus23 Can you restart the build again please? I think the screens got updated.

@Findus23
Copy link
Member

@fdellwing I think you’ll need to merge the latest 3.x-dev into your branch to fix the tests

Merge 3.x-dev into patch-1 (#1)
33a70c7 
* Fix one system test

* Removed totals

* Update TotalEvents.php

* Changed "right" positioning on ui-dialog hovering buttons (#12458)

* Improved Login and Reset Password Fields (#12448)

* Improved Login and Reset Password Fields

* Update index.twig

* Update userSettings.twig

* Rename the metrics

* Added CIDR Help (#12450)

Websites manager, exclude IP addresses, explain that CIDR notation is supported

* Added link to make accessibility to Embed guide website easier (#12475)

* Added link to make accessibility to Embed guide website

* Update index.twig

* Rough way of adding the page up and page down shortcuts to help list (#12461)

* Rough way of adding page up & down shortcut to help list

* To allow Macs to display different shortcut buttons

* Improved Print Versions (#12447)

* added print, hiding related reports in print version

* removed hover from showing in print version.

* Improved print version of report to remove next, view visitor profile. Added padding to the top and bottom of own visitor column

* Improved print version by removing search, add new page from print. Centered the page counter.

* Imroved print version by creating columns in visitor log instead of white space

* Removed box shadow from print version when hovered over

* Removed totals

* Update GetPageTitlesFollowingSiteSearch.php

* Update TotalEvents.php

* Update Events.php

* GeoIP re-attribution: debug output now shows changes to visits geo-location (#12481)

Re-create #12478 on the right branch

* Changed Feeds URL to HTTPS (#12449)

* Changed Feeds URL to HTTPS

* Fixed issue if an exception is called with https

* Update RssChangelog.php

* Fixed 2nd Feed URL

* Re alligned icon on metric display (#12464)

Align icon in Row Evolution popover

* Change Piwik => Matomo in PluginTemplates (#12482)

* Small performance improvement in custom tracker (#12443)

* Fix a possible issue when there are more than 2 billion visits tracked and system is 32 bits (#12469)

as reported in https://forum.matomo.org/t/piwik-log-visit-not-inserting-data-for-high-idvisit-value/26922/3

* Improvements to Annotations listing design (#12476)

* Improvements to Annotations listing design

:# Please enter the commit message for your changes. Lines starting

* fixed create a new annotation
 Please enter the commit message for your changes. Lines starting

* fixed the delete feature so it's not a button

* Fixes system tests and a bug

* Aligned all icons to left of menus (#12463)

* Aligned all icons to left of menus

* Minor tweaks for mobile view and horizontal alignment

* Modal Button Fix (#12462)

Modals popover can now be closed with keyboard

* Fixed maintenence mode to show a corresponding message based on record_statistics value (#12479)

* Fixed maintenence mode to show a corresponding message based on record_statics value

* Update message to "While the maintenance mode is active, data tracking is disabled."

* Fixed custom Dimension system tests

* Restore wording on Copy dashboard feature (#12483)

regressed in #12460

* Fixed selectors hugging left side of browser and search bar icon moving in Responsive View (#12470)

* Fixed top controls in Responsive View, fixed search bar icon moving

* fix typo

* Update layout.less

* Shows icon to disable/enable Zen Mode (#12459)

* Added icon to enable/disable Zen Mode

* Removed comment

* Update layout.less

* Changed arrow direction and location

* Adjusted Zen Mode to correspond to refreshing page

* Display issue on dashboard page - double icons

* Update zen-mode.js

* Update en.json

* Update layout.less

* Update CoreHome.php

* Fix tests (#12488)

* update expected system test file

* update UI files

* update test files

* add wait time

* update ui files

* move site selector loading indicator to the left (#12485)

* improve styling of shortcut help (#12486)

* improve styling of shortcut help

* make them a bit more square

* updates expected ui file
@tsteur
Copy link
Member

tsteur commented Jan 21, 2018

@mattab the smaller sparklines will be likely the same we experienced with Chromium and may be even a browser bug.

@fdellwing
Copy link
Contributor Author

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;
}

Maybe there is no script tag?

What should I do? I could give every div inside a sparkline the padding, but that could have some other side effects. Please decide that as you wish.

From my side, this PR is finished. The placeholder of the sparklines behaves weird, but it would be an really ugly hack to fix that in UI tests only.

piwik2

Should I do a clean PR without all with garbage?

@mattab
Copy link
Member

mattab commented Mar 19, 2018

Hi @fdellwing - sorry for the delay getting back. Yes, it would be great if you could create a new PR or update this one and only have the few files changed that you propose. We will review it for inclusion then. Thanks!

@fdellwing fdellwing mentioned this pull request Mar 19, 2018
Closed
@Findus23
Copy link
Member

@fdellwing I wouldn't worry too much about the Sparkline placeholders in the UI tests.

In #12066 I am removing them as creating the real Sparklines in the UI tests seems to work.

@fdellwing
Copy link
Contributor Author

I did a clean PR for this changes.

@fdellwing fdellwing closed this Mar 19, 2018
@fdellwing fdellwing deleted the patch-1 branch March 20, 2018 11:18
diosmosis pushed a commit that referenced this pull request Mar 21, 2018
@diosmosis
repush #12466
6e35f9a
diosmosis pushed a commit that referenced this pull request Mar 21, 2018
@diosmosis
repush #12466
3bb1061
diosmosis pushed a commit that referenced this pull request Mar 22, 2018
@diosmosis
repush #12466
3a5fb59
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
@mattab mattab modified the milestones: 3.5.0, 3.4.0 Mar 26, 2018
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
No reviews
Assignees
No one assigned
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@fdellwing @Findus23 @mattab @tsteur