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

Fix icon align of outlinks in datatables #16492

Merged
merged 18 commits into from Oct 15, 2020
Merged

Fix icon align of outlinks in datatables #16492

merged 18 commits into from Oct 15, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Sep 29, 2020

fixes #16436

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 29, 2020
@sgiehl sgiehl added this to the 4.0.0-RC milestone Sep 29, 2020
@tsteur
Copy link
Member

tsteur commented Sep 29, 2020

@sgiehl I think it needs a minor patch to align the icon when using the comparison feature? It seems now to high instead of too low

image

@mattab
Copy link
Member

mattab commented Sep 29, 2020

Could we backport this fix to 3.x, as it's a visual regression that almost everyone will notice, and some will use 3.x for another year or more?

@tsteur
Copy link
Member

tsteur commented Sep 29, 2020

@mattab we'll see. We might not even release another 3.X anytime soon and in general it's probably not too important.

@sgiehl
Copy link
Member Author

sgiehl commented Sep 30, 2020

@tsteur is that from the UI tests? Not sure why it's not positioned correctly there. But it looks correct locally in Chrome and Firefox 🤷

@tsteur
Copy link
Member

tsteur commented Sep 30, 2020

@sgiehl it looks fine in some reports when comparing (eg pages) but eg in "Websites" there seems to be a slight issue for me in my local install

image

also like in the tests in subtables the issue is happening
image

@sgiehl sgiehl force-pushed the iconalign branch 2 times, most recently from 4fee828 to d26fa70 Compare October 1, 2020 12:24
@sgiehl
Copy link
Member Author

sgiehl commented Oct 1, 2020

Ok. Hope everything looks good now.

Note: Bandwidth screenshots need to be updated before merging...

@tsteur
Copy link
Member

tsteur commented Oct 1, 2020

image
image

The icons still seem to be a bit off for me eg in websites report @sgiehl ? I made sure the CSS changes were applied by deleting the generated css file unless it's otherwise not applied. Can you have a look again maybe?

In pages report it needs to maybe one pixel down as well not sure but noticed when moving 1px it actually does nothing and setting a margin top of 2px is too much again so it might be fine

image

@mattab mattab mentioned this pull request Oct 2, 2020
@sgiehl
Copy link
Member Author

sgiehl commented Oct 2, 2020

@tsteur Then I guess we may need to refactor the whole HTML / CSS around the icons in datatables. Seems the position currently kind of depends on the rendering of the font. That's also the reason why it looks good on my machine, but slightly different in the UI tests.

That's how it looks for me on Windows (same for Chrome, Firefox and IE)
image

Could you maybe check how it looks if you remove the apple fonts from the active font list?

@tsteur
Copy link
Member

tsteur commented Oct 5, 2020

@sgiehl would it help to use font icons?

There are indeed differences depending on which font is used.

I wonder though why it used to work? There must be something that changed? I suppose could go back in versions to find out which version introduced the regression?

I was going to say maybe should no longer use system fonts by default and rather use a font that is available everywhere for a more consistent look across operating systems and browsers but finding a good font that is available everywhere might not be that easy either.

@Findus23
Copy link
Member

Findus23 commented Oct 5, 2020

I was going to say maybe should no longer use system fonts by default and rather use a font that is available everywhere for a more consistent look across operating systems and browsers but finding a good font that is available everywhere might not be that easy either.

I don't really think it is possible to use fonts that are the same on every operating system. Without webfonts, there is no font that is available on every OS. And even with webfonts on has the flash during loading of the font.

Personally, I also prefer fonts to look like users expect them and choose them (for their environment) rather than the same everywhere.

Maybe an alternative would be (inline) SVG files? They are pretty well-supported, but work more like images than fonts. If one doesn't want to duplicate the same SVG over and over, one could use defs and use like this: https://css-tricks.com/lodge/svg/13-svg-icon-system-use-element/

@sgiehl sgiehl force-pushed the iconalign branch 3 times, most recently from 44a68fd to 577a1d2 Compare October 5, 2020 18:24
@sgiehl
Copy link
Member Author

sgiehl commented Oct 5, 2020

I've now included the icons in the Matomo font. Hope it now also works on a mac.
The UI tests currently failing, are those from the submodules. Will need to update them before merging.

@tsteur
Copy link
Member

tsteur commented Oct 5, 2020

image

@sgiehl on Mac - Chromium it's now a pixel or two too high but at least the behaviour seems to be consistent across all reports, comparison feature, subtables. Would still need to move the icon though
image

Might also be good to use browserstack if needed?

@sgiehl
Copy link
Member Author

sgiehl commented Oct 6, 2020

@tsteur It still looks good on my side. So maybe it's still font related 🤷
btw. don't have access to browserstack

@tsteur
Copy link
Member

tsteur commented Oct 6, 2020

Sent you the login data @sgiehl maybe you can have a look there? Wonder though how it used to work. There must have been some change?

@sgiehl sgiehl force-pushed the iconalign branch 7 times, most recently from da62066 to a45b86c Compare October 8, 2020 14:40
@sgiehl sgiehl force-pushed the iconalign branch 4 times, most recently from dd38e36 to c0d487a Compare October 9, 2020 10:42
@tsteur
Copy link
Member

tsteur commented Oct 11, 2020

@sgiehl great. It looks definitely better now and great to see icons there instead of images.

The plus/minus icon looks still 1px too high.
image

This is also the case for me for all other fonts except for "sans-serif". Not sure what font Mac than is falling back to. I see though in the screenshots it seems to look fine (eg in https://media.githubusercontent.com/media/matomo-org/matomo/0e4626bf289fd16172ccbcbb787fe6923da34bef/plugins/Actions/tests/UI/expected-screenshots/ActionsDataTable_pageview_percentages.png). Although maybe it could also be moved one pixel further down if any possible? (In icomoon we could if needed move the icon one pixel down when generating the font AFAIK).

It's nicely aligned in the tests but wonder if it was possible to move one pixel down because mostly we would be having maybe lower case and the plus is quite a bit bigger than the lower letter?
image

Don't know what it looks like though on Linux/Windows.

fyi on mac with upper case letters
image

when using sans serif then it looks really well on Mac and wouldn't want to have it moved a pixel down. I assume this is the case for Windows/Linux too? So maybe we wouldn't want to move it down a pixel.

@sgiehl
Copy link
Member Author

sgiehl commented Oct 12, 2020

@tsteur I've moved the icons a pixel down in the font... Let me know if it's good to merge. Would then update the expected screenshots in the submodules before merging...

@tsteur
Copy link
Member

tsteur commented Oct 12, 2020

@sgiehl I'm still kind of having the same issue (100% cache was not used and using updated file). I think it moved it maybe slightly down. Like when I zoom in I can see at least the plus is on the same line as the font
image

but when viewing it in original size it's not 100% correct and still bit too high. I don't know if it can be moved again a bit further down or so?

image
image

I wonder actually if a different font has a better icon. This icon doesn't seem to be as much suited for being displayed so small. Even on a retina display it looks a bit pixelated due to some aliasing or so
image

On my regular screen the icon looks quite blurry
image

It's not as much visible in a screenshot I think. Other fonts might have the same problem though considering we show it so small (and my regular screen is not the best but users might have similar screens).

Tested some other icons like from material design
image

but they have issue too see above regular screen vs below retina (looking a lot better)
image

Is there maybe some font smoothing or so that can be disabled to make this better or maybe something needs to be enabled?

Funny thing: The old image that we were using looks good on my regular screen but bad on the retina screen :)

It might not be too important for now just thought I mention it. I don't know what it looks like on Windows/Linux and whether it can be again moved down another pixel the plus / minus icon. The outlink is fine and not needs changing

@sgiehl
Copy link
Member Author

sgiehl commented Oct 13, 2020

On windows the align is actually fine for the plus/minus icon. I've added some additional css for the icon to improve the smoothing... Can't see a difference on Windows, but maybe it helps for Mac

@tsteur
Copy link
Member

tsteur commented Oct 13, 2020

Doesn't really change anything here. I'm thinking the icon might not be best but I guess most other icons would have same problem since it's shown so small. Could we maybe show the icon bigger? Like below (font-size:16px). In my case it even improves the position of the icon a bit. I guess the current 13px results in a lot of "rounding/smoothing" and is generally not as much visible.

image

compared to standard 13px
image

@sgiehl
Copy link
Member Author

sgiehl commented Oct 14, 2020

increasing the icon size sounds fine. will update the css and expected screenshots...

@tsteur
Copy link
Member

tsteur commented Oct 14, 2020

That works well on Mac 👍 not sure what it's like on Windows or Linux but for me it looks good to merge

@sgiehl sgiehl merged commit 81e5f36 into 4.x-dev Oct 15, 2020
@sgiehl sgiehl deleted the iconalign branch October 15, 2020 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outlinks icons are not aligned in all reports
4 participants